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

make-derivation.nix: warn that meta.available is deprecated #227342

Closed
wants to merge 5 commits into from
Closed

make-derivation.nix: warn that meta.available is deprecated #227342

wants to merge 5 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 20, 2023

Description of changes

Deprecate meta.available and recommend lib.meta.availableOn stdenv.hostPlatform instead.

meta.available is affected by impure environment variables like NIXPKGS_ALLOW_NONSOURCE. It's okay for environment variables like that to cause a build to fail, but if we start switching dependencies in and out based on meta.available we can get builds that succeed differently when you flip those on.

This actually happened to me shortly after I started using allowNonSource=false, and it was a total nightmare to debug: #179648

Despite the similar-sounding name, lib.meta.availableOn doesn't have this problem. It is unaffected by impure environment variables and config.allowXYZ preferences. It only checks meta.{platforms,badPlatforms}.

#227120 (comment)

Also adds three commits which change the (only) three uses of meta.available in nixpkgs to use lib.meta.availableOn instead.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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.

Adam Joseph added 3 commits April 20, 2023 14:21
pkg.meta.available is deprecated (see subsequent commit which adds a
warning) and does not distinguish between the buildPlatform and the
hostPlatform.
pkg.meta.available is deprecated (see subsequent commit which adds a
warning) and does not distinguish between the buildPlatform and the
hostPlatform.
pkg.meta.available is deprecated (see subsequent commit which adds a
warning) and does not distinguish between the buildPlatform and the
hostPlatform.
@ghost ghost requested review from Ericson2314 and matthewbauer as code owners April 20, 2023 21:30
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Apr 20, 2023
@ghost ghost requested review from alyssais and sternenseemann April 20, 2023 21:34
@ghost ghost requested a review from piegamesde as a code owner April 20, 2023 22:13
Deprecate `meta.available` and recommend `lib.meta.availableOn
stdenv.hostPlatform` instead.

`meta.available` is affected by impure environment variables like
NIXPKGS_ALLOW_NONSOURCE. It's okay for environment variables like
that to cause a build to fail, but if we start switching
dependencies in and out based on meta.available we can get builds
that succeed differently when you flip those on.

This actually happened to me shortly after I started using
allowNonSource=false, and it was a total nightmare to debug:
#179648

Despite the similar-sounding name, lib.meta.availableOn doesn't have
this problem. It is unaffected by impure environment variables and
config.allowXYZ preferences.

#227120 (comment)
@sternenseemann
Copy link
Member

Can you figure out why ofborg and Hydra evaluate meta.available? My intuition would be that this should not be evaluated normally, so the inOfBorg hack would not be necessary?

I think this change is best to have in after branch-off to give downstream and us a bit of time to deal with any unforeseen consequences. A changelog entry is also a good idea.

@ghost
Copy link
Author

ghost commented Apr 24, 2023

Can you figure out why ofborg and Hydra evaluate meta.available?

I think it's this line in hydraJob:

lib.deepSeq drv' drv';

Another alternative that I came up with after writing this would be for checkMeta to abort instead of throw when it encounters something forbidden by NIXPKGS_ALLOW_*. This ensures that NIXPKGS_ALLOW_* can prevent eval from succeeding but can't affect the result of a successful eval (because you can't "catch" an abort using tryEval).

@ghost
Copy link
Author

ghost commented Apr 24, 2023

this change is best to have in after branch-off

Yes definitely. This is a candidate for the "okay to break stuff" window right after the release.

@ghost ghost marked this pull request as draft April 24, 2023 02:17
@sternenseemann
Copy link
Member

As far as I can tell, hydra-eval-jobs never looks at meta.available, so it should be possible to builtins.removeAttrs meta [ "available" ] in hydraJob to circumvent this problem.

@ghost ghost closed this Oct 22, 2023
@ghost ghost deleted the pr/deprecate/meta.available branch October 22, 2023 07:38
This pull request was closed.
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.

1 participant