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

neovim-unwrapped: Fix 'disallowedRequisites' error #330026

Closed
wants to merge 1 commit into from

Conversation

Gerg-L
Copy link
Contributor

@Gerg-L Gerg-L commented Jul 25, 2024

Description of changes

Tested on nix 2.24pre and nixVersions.minimum (2.3.18)

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

@Gerg-L
Copy link
Contributor Author

Gerg-L commented Jul 25, 2024

Not sure this is the correct answer, i can't get this build to fail with CA derivations which was the original reason for the postInstall patch at all

@Gerg-L
Copy link
Contributor Author

Gerg-L commented Jul 26, 2024

it looks like the disallowedRequisites came from 9305ed3
But with removing the disallowedRequisites and the postInstall
it builds fine with nix-build --arg config '{ contentAddressedByDefault = true; }' -A neovim-unwrapped
CC: @tie you added these bits originally can you look at this?

@tie
Copy link
Member

tie commented Jul 26, 2024

CC: @tie you added these bits originally can you look at this?

👀

I'll try to take a look at this tomorrow.

We should probably keep removeReferences though since IIRC neovim embeds the path to the compiler used to build it (e.g. you can verify this with nix-tree).

@Gerg-L
Copy link
Contributor Author

Gerg-L commented Jul 26, 2024

alright if that's the case eedc7c0 should work, i'm not sure on the exact correct nix version i believe it's 2.18 that outputChecks.<output>.disallowedReferences was added but i'm not entirely sure

@tie
Copy link
Member

tie commented Jul 26, 2024

Maybe we can set __structuredAttrs = true and outputChecks unconditionally (without disallowedRequisites and version checks)? 🤔
I assume this should work as expected for recent Nix versions that support structured attributes and be no-op otherwise.

Converting {,dis}allowed{References,Requisites} to outputChecks based on the the Nix version should probably be done in mkDerivation and not on per-package basis. Or ideally shouldn't be done at all, I'm not really a fan of discarding string context in these attributes as is currently implemented since it breaks with CA derivations, hence the PR you've mentioned—and removeReferences/nukeReferences should be enough to avoid unwanted references in the derivation outputs.

@Gerg-L
Copy link
Contributor Author

Gerg-L commented Jul 27, 2024

your suggested solution works on

  • 2.3
  • 2.18
  • 2.24-pre
  • 2.24-pre CA

@Gerg-L Gerg-L marked this pull request as ready for review July 27, 2024 00:04
@Aleksanaa Aleksanaa changed the base branch from master to staging August 3, 2024 15:17
@Aleksanaa Aleksanaa requested review from tie and removed request for tie August 3, 2024 15:18
@Aleksanaa
Copy link
Member

Sorry

@tie
Copy link
Member

tie commented Aug 19, 2024

Apparently a similar fix was merged in #334915 (cc @Ma27). Is it OK to close this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants