lib/systems/parse.nix: replicate cygwin/msvc parsing hack when unparsing #235229
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For almost a decade nixpkgs has parsed `*-*-{cygwin,msvc}` as being
ABIs for a common kernel (windows), rather than as kernels. It
isn't really reasonable to change nixpkgs' handling at this point,
so this commit replicates the special-case hack when unparsing, in
order to ensure that `(unparse . parse)==id`.
ghost
requested review from
alyssais,
Ericson2314 and
matthewbauer
as code owners
ghost
mentioned this pull request
1 task
Ericson2314
requested changes
Aug 16, 2023
There was a problem hiding this comment.
As I wrote in the main PR (I missed this one before, whoops) I don't think we should do this.
windows-gnu I already fixed in GNU config, and with https://lists.gnu.org/archive/html/config-patches/2023-08/msg00011.html window-cygnus will be accepted too.
I would rather we keep on normalizing things in the way we do, and instead add these cases as tests where we explicitly choose to normalize in a different way (like x86_64-linux-gnu).
After a deprecation cycle, and fixing any downstream bugs in GCC etc., we can stop accepting ...-cygwin and ...-mingw32 if you like, so our deviation from GNU config is reduced.
Ericson2314
requested changes
Aug 16, 2023
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
For about five years nixpkgs has parsed
*-*-{cygwin,msvc}as ifcygwinandmsvcwere ABIs for a common kernel (windows), rather than as kernels (which is howgnu-configrather counterintuitively treats them). This PR replicates the special-case parsing hack when unparsing, in order to ensure that(unparse . parse)==id.These windows triples are probably used much more often in Nix expressions downstream of nixpkgs than in nixpkgs itself -- so we have really poor visibility on what kind of breakage would be caused by changing this treatment. Because most of the uses of this behavior are by now long-established and out-of-tree where we can't see them, I think it would be a bad idea to try to change this, and we should just grandfather the hack as a special exception.
Part of:
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/)