treewide: expunge functions from platforms #238331
Conversation
ghost
mentioned this pull request
github-actions
bot
added
6.topic: emacs
ghost
marked this pull request as ready for review
ghost
requested review from
jtojnar,
adisbladis,
matthewbauer,
RaitoBezarius,
cdepillabout,
sternenseemann,
maralorn,
dasJ,
Ericson2314 and
alyssais
as code owners
ofborg
bot
added
8.has: clean-up
10.rebuild-darwin: 0
There was a problem hiding this comment.
Excellent!
- With 5d8e668d16f7ce7c32399007d3a49a27322ac3ed, the issue described in stdenv: fix crossSystem localSystem comparison #237427 should disappear and we'd be able to revert
lib.systems.equals#237512 for good (and live worriless when the functions are finally removed after a deprecation period of one or two releases, I guess). - We should add polyfills to 23.11, so backporting cross changes becomes easier. These would have the form:
canExecute = machinePlatform: machinePlatform.canExecuteetc. This wouldn't break anything, but would ideally make backported changes just work (as long as they apply)
figsoda
added
the
12.approvals: 1
| canExecute = canExecutePlaceholder final; | ||
|
|
||
| # 2023-06-17: `platform.emulatorAvailable` has been removed in favor of `lib.systems.emulatorAvailable platform` | ||
| emulatorAvailable = emulatorAvailablePlaceholder final; |
There was a problem hiding this comment.
These also don't need to be placeholders, right?
There was a problem hiding this comment.
These were added by @sternenseemann here: #238331 (review) in commit 5d8e668d16f7ce7c32399007d3a49a27322ac3ed
They do need to be placeholders, because they are functions. And Nix's definition of equality for functions is crackheaded.
There was a problem hiding this comment.
See 5d8e668#diff-2165823a8d82c5dd1353601bd290df8bd431f9ee2096750d9ef655cf5d251998R37-R47
# These definitions have to be floated out into a `let … in` binding to ensure
# that they are the same exact value struct in the evaluating Nix
# implementation. Due to a quirk in how equality is implemented, values including
# functions are considered equal if they have the same memory location as long as
# they are inside a container value and not compared directly. Thus any attribute
# set will compare equal to itself even if it contains functions.
# By floating the functions out of the attribute set construction we'll ensure
# that the functions have the same memory location regardless of what call to
# lib.systems.elaborate produced them. This ensures that platform sets will be
# equal even if they have been constructed by different calls to lib.systems.elaborate
# —as long as their other contents are the same.
ghost
marked this pull request as draft
|
Rebased |
ghost
marked this pull request as ready for review
ofborg
bot
added
2.status: merge conflict
|
Rebased, squashed. |
lib/systems/default.nix
Outdated
| parse = import ./parse.nix { inherit lib; }; | ||
|
|
||
| # uncomment after 23.11 branch-off |
There was a problem hiding this comment.
To be kind to external users supporting both stable and unstable, these shouldn't be made warnings until after 24.05 is branched off, and errors after 24.11 is branched off, right?
There was a problem hiding this comment.
To be kind to external users supporting both stable and unstable, these shouldn't be made warnings until after 24.05 is branched off, and errors after 24.11 is branched off, right?
I was actually hoping to get the lib.warn in before the 23.11 branch-off, since lib.warn is only a hard CI failure for nixpkgs itself.
Since lib.warn is our way of giving a heads-up to people downstream of us, anybody who has their CI set up to fail if nixpkgs emits a warning is basically turning anything sent across our only communication channel into breakage. I think that's a bad choice and I don't think many users choose it (for very long).
So ideally I'd like to put the lib.warn in before 23.11 branch-off.
The timeline for the removal of these attributes is less urgent; if that needs to wait until after 23.11 I guess that's okay. Keep in mind however that until these attributes are removed, we have no way to lib.warn on any deprecated attributes of system objects (like isEfi), because removeFunctions needs to use lib.isFunction and there is no way to write it which is (AFAICT) lazy enough to not emit the warning. So any deprecations of platform attributes cause nixpkgs CI to fail.
Ultimately, if people really want even the lib.warn to wait until after 23.11, then I guess it has to wait.
Even though this PR has two approvals I will not merge it myself.
Edit: oh, I see, if this PR merges as-is there is no way to write code that works and does not emit warnings on both 23.05 and the post-merge-of-this-PR tree.
Okay I will update this to account for that.
There was a problem hiding this comment.
Alright, let me know if 1f7f6d396241c54829df4209c383a1aa2062c35a captures this intent correctly.
ghost
marked this pull request as draft
ghost
marked this pull request as ready for review
This commit moves `canExecute`, `emulator`, and `emulatorAvailable`
out of `{host,build,target}Platform` and lifts them into
`lib.systems`. Stubs have been left behind to assist with migration
of out-of-tree code.
Nix's function equality is unpredictable and error-prone; we should
not rely on it. Since we need to be able to test
`{host,build,target}Platform` attrsets for equality, the most
sustainable long-term solution is to forbid functions from these
attrsets, much like we do for the `meta` attrset.
The stubs which have been left behind are a narrowly-defined case
where Nix's function equality *does* behave predictably.
Comparing platform sets using `==` works for most cases since the in the native case, the platform sets would not only be equal, but the same identical value (in terms of memory location). This is, however, technically not always the case, e.g. when specifying crossSystem explicitly, cases can arise when we should use a native stdenv, but don't since the functions wouldn't compare as equal due to them being constructed in separate invocations of `lib.system.elaborate`. This problem is described in #237427 and motivated #237512 as an equality predicate that ignors the functions. Since the functions in the platform attribute set no longer relate to the platform set at all, we can use the same function value for every placeholder deprecation error function in the attribute set—consequently they will all compare as equal even for independently constructed platform sets. This is achieved by floating the funtions into a let binding in `lib/systems/default.nix`, ensuring that they are only constructed once per nixpkgs instance, i.e. when that file is imported. (Consequently, comparison of platform sets should be reliable as long as they come from the same instance of nixpkgs.) let inherit ( import <nixpkgs> { localSystem = "x86_64-linux"; crossSystem.system = "x86_64-linux"; } ) buildPlatform hostPlatform ; in buildPlatform == hostPlatform # => true This property is also verified with new test cases.
ghost
marked this pull request as draft
|
Fixed merge conflict. |
ghost
marked this pull request as ready for review
This commit moves `canExecute`, `emulator`, and `emulatorAvailable`
out of `{host,build,target}Platform` and lifts them into
`lib.systems`. Stubs have been left behind to assist with migration
of out-of-tree code. These will be changed to `lib.warn` or
`throw`, but that change will not be backported to 23.05.
Nix's function equality is unpredictable and error-prone; we should
not rely on it. Since we need to be able to test
`{host,build,target}Platform` attrsets for equality, the most
sustainable long-term solution is to forbid functions from these
attrsets, much like we do for the `meta` attrset.
There was a problem hiding this comment.
We should move the first date a bit, because for a month we support all three versions: 23.05, 23.11, unstable.
Maybe it'd be more clear to make the 23.05 EOL changes in a draft PR instead of commented code.
Perhaps link the PR in a comment if you still want to provide context for readers of the code.
| emulatorPlaceholder = _: throw "2023-06-17: `platform.emulator` has been removed in favor of `lib.systems.emulator platform`"; | ||
| */ | ||
|
|
||
| # uncomment after 23.11 branch-off |
There was a problem hiding this comment.
| # uncomment after 23.11 branch-off | |
| # uncomment in January 2024, after 23.05 EOL |
| */ | ||
|
|
||
| # uncomment after 23.11 branch-off | ||
| # delete after 24.05 branch-off |
There was a problem hiding this comment.
| # delete after 24.05 branch-off | |
| # delete canExecutePlaceholder, emulatorAvailablePlaceholder and emulatorPlaceholder after 24.05 branch-off (throwing would have been nice, but throws off `==` equality when comparing platforms elaborated on distinct but compatible Nixpkgs source trees) |
- Won't be grouped by the comment syntax anymore, so be explicit
- Explain why not throw in this case
| @@ -64,6 +64,33 @@ lib.runTests ( | |||
| testunix = mseteq unix (linux ++ darwin ++ freebsd ++ openbsd ++ netbsd ++ illumos ++ cygwin ++ redox); | |||
| }) | |||
|
|
|||
|
|
|||
| # Uncomment after 23.11 | |||
There was a problem hiding this comment.
I think this needs to be in sync with the other uncomment comment.
| # Uncomment after 23.11 | |
| # Uncomment after 23.05 EOL in January 2024 |
Cc: @roberth @Artturin @sternenseemann
Description of changes
This commit moves
canExecute,emulator, andemulatorAvailableout of{host,build,target}Platformand lifts them intolib.systems. Stubs have been left behind to assist with migration of out-of-tree code.Nix's function equality is unpredictable and error-prone; we should not rely on it. Since we need to be able to test
{host,build,target}Platformattrsets for equality, the most sustainable long-term solution is to forbid functions from these attrsets, much like we do for themetaattrset.Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)