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

autoconf-archive: fix quoting of m4_fatal #355948

Merged
merged 1 commit into from
Nov 17, 2024
Merged

Conversation

corngood
Copy link
Contributor

@corngood corngood commented Nov 14, 2024

This fixes the cava package, and could potentially fix others. I tested this as an override in cava and it worked, but I haven't built this in staging.

I believe it was broken by 59b91ba.

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.

@jopejoe1 jopejoe1 added backport staging-24.11 Backport PR automatically 0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign labels Nov 14, 2024
@jopejoe1
Copy link
Member

Getting a hash mismatch

error: hash mismatch in fixed-output derivation '/nix/store/a0avdgs7l859rlxcb2z6y63a5kq5fsc4-ax_check_gl-fix.patch.drv':
        likely URL: https://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=patch;h=427e226a2fe3980388abffd6de25ed6b9591cce3
         specified: sha256-ajmyhz7boaRsM6wfLuKkZMo6pC53FwABX6p6ZcsoMV8=
            got:    sha256-gQbfXqfhpjjsUek6A4xqmnHdKPayhZhcXk6o4O+7Iqo=

@corngood
Copy link
Contributor Author

Ah, sorry, that was because I started with an override using fetchpatch, and changed it to fetchurl + explicit name here. Only the name would have differed. Fixed.

Copy link
Member

@jopejoe1 jopejoe1 left a comment

Choose a reason for hiding this comment

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

Tested building a few packages with this.

@@ -9,6 +9,15 @@ stdenv.mkDerivation rec {
hash = "sha256-e81dABkW86UO10NvT3AOPSsbrePtgDIZxZLWJQKlc2M=";
};

patches = [
# fix https://bugs.gentoo.org/941845
(fetchurl {
Copy link
Member

Choose a reason for hiding this comment

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

what is the reason for using fetchurl here? fetchpatch/fetchpatch2 should be preferred when fetching patches generated by git frontends

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe there's some bootstrap context where this package is evaluated, but fetchpatch is broken. I think because the bootstrap fetchurl is incompatible with it? It's easy to repro by evaluating this package for linux with the change.

Copy link
Member

Choose a reason for hiding this comment

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

If there would be a circular dependency with a fetcher, we generally include patches directly in Nixpkgs. Using fetchurl for patches means that the patch might become unreproducible if the upstream service changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this to include the patches in nixpkgs. There were two other similar patches that were merged together, so I grabbed them all.

I think that's a good idea given these are small and it's a critical package.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 15, 2024
@corngood corngood changed the title autoconf-archive: fix error in ax_check_gl autoconf-archive: fix quoting of m4_fatal Nov 17, 2024
This cherry-picks a patch set fixing calls to m4_fatal which caused some
packages to fail in autoconf (e.g. cava).
@corngood
Copy link
Contributor Author

In gentoo they apparently just hacked cava to work-around this: https://bugs.gentoo.org/941845

Given how infrequently autoconf-archive is updated, and the possibility of this breaking other packages, I think it still might be a good idea to fix it here.

@vcunat vcunat merged commit f4acddd into NixOS:staging Nov 17, 2024
10 of 11 checks passed
Copy link
Contributor

Successfully created backport PR for staging-24.11:

@Hisui02
Copy link

Hisui02 commented Nov 20, 2024

When is this going to be available in unstable branch?

@vcunat
Copy link
Member

vcunat commented Nov 21, 2024

My hope is that this weekend we can get it into 24.11 and soon afterwards into master (nixos-ustable, etc.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 12.approvals: 1 This PR was reviewed and approved by one reputable person backport staging-24.11 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants