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

tone: 0.1.5 -> 0.2.3 #364325

Merged
merged 8 commits into from
Dec 17, 2024
Merged

tone: 0.1.5 -> 0.2.3 #364325

merged 8 commits into from
Dec 17, 2024

Conversation

jwillikers
Copy link
Contributor

Clean up the recipe, including a version check and update script.

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.

@jwillikers jwillikers requested review from jvanbruegge, mdarocha and natsukium and removed request for jvanbruegge and mdarocha December 11, 2024 19:25
@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 11, 2024
pkgs/by-name/to/tone/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/to/tone/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/to/tone/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/to/tone/update.sh Outdated Show resolved Hide resolved
@jwillikers jwillikers changed the title tone: 0.1.5 -> 0.2.1 tone: 0.1.5 -> 0.2.2 Dec 13, 2024
@gepbird gepbird mentioned this pull request Dec 13, 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.

Changes LGTM and tested the binary. Looking at the releases this seems safe to backport.

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 364325


x86_64-linux

✅ 1 package built:
  • tone

@gepbird gepbird added the backport release-24.11 Backport PR automatically label Dec 13, 2024
@gepbird gepbird requested a review from corngood December 13, 2024 22:03
@jwillikers jwillikers changed the title tone: 0.1.5 -> 0.2.2 tone: 0.1.5 -> 0.2.3 Dec 14, 2024
@jwillikers
Copy link
Contributor Author

Updated to 0.2.3

@github-actions github-actions bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1 labels Dec 14, 2024
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 for opening the issue and updating the package, upstream resolved it really fast!

rev = "v${version}";
sha256 = "sha256-HhXyOPoDtraT7ef0kpE7SCQbvGFLrTddzS6Kdu0LxW4=";
hash = "sha256-fe0fcX617vsRdu42FEan181Zh5Todt9FLtOuD+C7yqA=";
Copy link
Contributor

@gepbird gepbird Dec 14, 2024

Choose a reason for hiding this comment

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

I got a hash mismatch

Most likely upstream deleted and made a new 0.2.3 release sandreas/tone#77 (comment)

error: hash mismatch in fixed-output derivation '/nix/store/9y5bgmn5il6fpwkgzhwi8sld7i6fpvi9-source.drv':
        likely URL: https://github.com/sandreas/tone/archive/v0.2.3.tar.gz
         specified: sha256-fe0fcX617vsRdu42FEan181Zh5Todt9FLtOuD+C7yqA=
            got:    sha256-NBFAPEeUKZgyfNlvcOBS1IpktEnI+fOd9WLj0ByzpLY=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that's what happened. Should be good now.


dotnetInstallFlags = [
"-p:PublishSingleFile=false"
];

dotnet-sdk = dotnetCorePackages.sdk_6_0;
Copy link
Contributor

@gepbird gepbird Dec 14, 2024

Choose a reason for hiding this comment

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

Even though currently .NET 8 is the default sdk, I think we should be explicit and keep dotnet-sdk and dotnet-runtime attrs (we can drop the extra dotnet flag that forces a TargetFramework). It could be useful for example when .NET 8 will become EOL and .NET 10 will be the default: instead of automatically upgrading to incompatible 10, this package would still work but will be marked insecure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great to know! I'll keep that in mind for C# packages from now on.

Comment on lines 42 to 45
platforms = with lib.platforms; linux ++ darwin ++ windows;
broken = !stdenv.hostPlatform.isLinux;
Copy link
Contributor

@gepbird gepbird Dec 14, 2024

Choose a reason for hiding this comment

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

Interesting pattern, and make sense since upstream supports these platforms and we only tested the linux build, but I haven't seen packages being marked as broken without someone observing the failure.

Nit:

Suggested change
platforms = with lib.platforms; linux ++ darwin ++ windows;
broken = !stdenv.hostPlatform.isLinux;
platforms = with lib.platforms; linux ++ darwin ++ windows;
broken = !stdenvNoCC.isLinux;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I think that platforms is supposed to represent all the platforms a package supports. I don't really have a good way test on a mac, and I don't even want to think about trying to run Nix on Windows, which is why I did it this way. I was recommended this approach when submitting either the wlink or wchisp package, since Linux was all I'd confirmed as working.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tested this on both darwin archs, and it works at least enough to do --help and --version.

I think we should remove broken and let it be added if/when actual problems are discovered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for testing, I've updated this as suggested.


src = fetchFromGitHub {
owner = "sandreas";
repo = pname;
repo = "tone";
rev = "v${version}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit

Suggested change
rev = "v${version}";
rev = "refs/tags/v${version}";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jwillikers jwillikers force-pushed the tone-2.0.1 branch 2 times, most recently from 6ba9006 to 51e8b9f Compare December 15, 2024 01:41
@jwillikers jwillikers force-pushed the tone-2.0.1 branch 2 times, most recently from 8fa03fe to 8e0867f Compare December 15, 2024 01:45
@jwillikers jwillikers requested a review from gepbird December 15, 2024 03:35
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, LGTM!

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 364325


x86_64-linux

✅ 1 package built:
  • tone

@gepbird gepbird added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 15, 2024
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1 10.rebuild-linux: 1 labels Dec 15, 2024
@Aleksanaa
Copy link
Member

@ofborg build tone

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Dec 16, 2024
@corngood corngood merged commit 2c61f5d into NixOS:master Dec 17, 2024
43 of 44 checks passed
@nix-backports
Copy link

nix-backports bot commented Dec 17, 2024

Successfully created backport PR for release-24.11:

@jwillikers jwillikers deleted the tone-2.0.1 branch December 17, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people backport release-24.11 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants