Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lumafly: use dotnet sdk 9 as sdk 7 has been marked insecure #360640

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

rohanssrao
Copy link
Contributor

This is a temporary patch until TheMulhima/Lumafly#134 is merged and a release is made.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 1, 2024
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-linux: 1 labels Dec 1, 2024
@gepbird gepbird mentioned this pull request Dec 2, 2024
17 tasks
Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, changes LGTM and GUI opens.

Some nits, feel free to ignore if you feel it is not important or out of scope:

  • currently the bin folder not only contains Lumafly, but a bunch of DLLs that doesn't need to be there and can cause conflicts with other packages having the same DLLs which is not that unlikely. To fix it you can add this: executables = [ "Lumafly" ];
  • format the changed file: nix run nixpkgs/nixos-unstable#nixfmt-rfc-style -- -- pkgs/by-name/lu/lumafly/package.nix
  • the desktop item's comment references a meta attribute, in case someone changes meta.description (eg. in a treewide commit affecting many 100s of packages), this package would be rebuilt which shouldn't happen. Consider hard coding it
  • in fetchFromGitHub's rev specify that we expect a git tag: rev = "refs/tags/v${version}";

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 360640


x86_64-linux

✅ 1 package built:
  • lumafly

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 4, 2024
@rohanssrao
Copy link
Contributor Author

@gepbird I made the edits, thanks!
I'm not sure how to squash my commits.

$ git clone -b --depth=1 git@github.com:rohanssrao/nixpkgs.git && cd nixpkgs
$ git rebase -i HEAD~3
fatal: invalid upstream 'HEAD~3'
$ git log
commit f37ceed67a971f506fac3dbc3c3f100d979f36be (grafted, HEAD -> patch-1, origin/patch-1)
Author: rohanssrao <rohanssrao@gmail.com>
Date:   Wed Dec 4 21:18:07 2024 -0500

    Merge branch 'NixOS:master' into patch-1

It seems like I can only see the last commit.

@gepbird
Copy link
Contributor

gepbird commented Dec 5, 2024

Thanks, changes LGTM but I'll test it later.

It seems like I can only see the last commit.

That's what --depth 1 does, it only dowloads the last commit which is useful if you don't need the previous history as it can save a lot of disk space, but in this case we need that :)

@rohanssrao
Copy link
Contributor Author

rohanssrao commented Dec 5, 2024

@gepbird Appreciate it! I guess I shouldn't have synced the fork between my commits because I had to clone to a depth of 230 which took a while lol.
I did git reset --hard <first_commit>, made my changes again, then did

git add pkgs/by-name/lu/lumafly/package.nix
git commit --amend --no-edit
git push --force

and that worked!

Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 360640


x86_64-linux

✅ 1 package built:
  • lumafly

Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for bothering again with extra nits and suggestions, again feel free to ignore:

  • you could add an passthru.updateScript, this should be sufficient:
#!/usr/bin/env nix-shell
#!nix-shell --pure -i bash -p bash nix nix-update git cacert
set -eo pipefail

nix-update lumafly
$(nix-build . -A lumafly.fetch-deps --no-out-link)

@rohanssrao rohanssrao force-pushed the patch-1 branch 2 times, most recently from 7346da1 to 3f65a0c Compare December 11, 2024 14:08
@rohanssrao
Copy link
Contributor Author

Done!

@gepbird gepbird requested a review from corngood December 11, 2024 15:12
@corngood
Copy link
Contributor

nix-update lumafly
$(nix-build . -A lumafly.fetch-deps --no-out-link)

@gepbird it occurred to me after we discussed this, that maybe it was a bad idea. Will this result in fetch-deps running even when the packages is already up-to-date?

@gepbird
Copy link
Contributor

gepbird commented Dec 11, 2024

nix-update lumafly
$(nix-build . -A lumafly.fetch-deps --no-out-link)

@gepbird it occurred to me after we discussed this, that maybe it was a bad idea. Will this result in fetch-deps running even when the packages is already up-to-date?

Unfortunately yes :(

Should we stick the the old method where we get the current and latest version, exit if they are the same, else run update-source-version (or nix-update) and the fetch-deps script or do you have a nicer idea?

@corngood
Copy link
Contributor

Unfortunately yes :(

Running fetch-deps all the time is not going to break updates, it's just a waste of resources for whoever is running the automated update jobs. I don't know how that infrastructure works, so I'm not sure how big a problem it is.

Should we stick the the old method where we get the current and latest version, exit if they are the same, else run update-source-version (or nix-update) and the fetch-deps script or do you have a nicer idea?

I think we could get the version from the derivation before and after calling nix-update and then run fetch-deps if it changes. That would be pretty simple to implement, and it wouldn't require duplicating any of the actual update logic (interacting with the source repo, etc).

I'd need to do some investigation to know if a better way is possible.

@corngood
Copy link
Contributor

corngood commented Dec 12, 2024

@rohanssrao could we do this:

diff --git a/pkgs/by-name/lu/lumafly/package.nix b/pkgs/by-name/lu/lumafly/package.nix
index 8a05f70ba1c0..910ab26ab4bf 100644
--- a/pkgs/by-name/lu/lumafly/package.nix
+++ b/pkgs/by-name/lu/lumafly/package.nix
@@ -39,8 +39,10 @@ buildDotnetModule rec {
     #!nix-shell --pure -i bash -p bash nix nix-update git cacert
     set -eo pipefail
 
+    prev_version=$(nix eval --raw -f. lumafly.version)
     nix-update lumafly
-    $(nix-build . -A lumafly.fetch-deps --no-out-link)
+    [[ $(nix eval --raw -f. lumafly.version) == "$prev_version" ]] ||
+      $(nix-build . -A lumafly.fetch-deps --no-out-link)
   '';
 
   runtimeDeps = [

to avoid running fetch-deps when the package is up to date? I did some testing and it seems to work okay.

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 12, 2024
@rohanssrao
Copy link
Contributor Author

Done!

@corngood corngood added the backport release-24.11 Backport PR automatically label Dec 13, 2024
@corngood corngood merged commit 59ab33c into NixOS:master Dec 13, 2024
24 of 25 checks passed
@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented Dec 13, 2024

Successfully created backport PR for release-24.11:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes backport release-24.11 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants