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

lib/system: move toLosslessStringMaybe into lib/tests #239132

Merged
1 commit merged into from Jun 23, 2023
Merged

lib/system: move toLosslessStringMaybe into lib/tests #239132

1 commit merged into from Jun 23, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jun 22, 2023

Description of changes

toLosslessStringMaybe is not used by anything other than lib/tests, so it can be private to that file.

I don't think this function was terribly well thought-through.

  1. If people start using it, we will become permanently dependent on the ability to test platforms for equality.
  2. It makes the elaboration process more fragile, because it encourages code outside of nixpkgs to become sensitive to the minute details of how elaboration happens.
  3. It turns platforms for which toLosslessStringMaybe plat == null into second-class citizens. Our long term direction should be away from assuming that a platform is just a string -- places where we make that assumption already cause massive headaches for pkgsStatic and pkgsLLVM whose stdenv.hostPlatforms both fall into this category.
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.11 Release Notes (or backporting 23.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.

toLosslessStringMaybe is not used by anything other than lib/tests,
so it can be private to that file.

I don't think this function was terribly well thought-through.  If
people start using it, we will become permanently dependent on the
ability to test platforms for equality.  It also makes the
elaboration process more fragile, because it encourages code outside
of nixpkgs to become sensitive to the minute details of how
elaboration happens.
@alyssais
Copy link
Member

Should we do a deprecation period for lib.systems.toLosslessStringMaybe? It might have out-of-tree users.

@ghost
Copy link
Author

ghost commented Jun 23, 2023

Should we do a deprecation period for lib.systems.toLosslessStringMaybe? It might have out-of-tree users.

It was added just last week in 5ff6f51

@ghost ghost merged commit 35480b6 into NixOS:master Jun 23, 2023
@ghost ghost deleted the pr/toLosslessStringMaybe/unexpose branch June 23, 2023 09:40
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