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

treewide: throw on unsupported system src access #301704

Merged
1 commit merged into from Apr 5, 2024
Merged

treewide: throw on unsupported system src access #301704

1 commit merged into from Apr 5, 2024

Conversation

ghost
Copy link

@ghost ghost commented Apr 5, 2024

Description of changes

throw rather than abort when accessing the src attribute when system is not supported

testing: accessed src attribute with unsupported system and verified throw (caught with tryEval).

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.05 Release Notes (or backporting 23.05 and 23.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.

Add a 👍 reaction to pull requests you find important.

@lolbinarycat
Copy link
Contributor

actually, why not factor this off into a lib function? that way if we want to make it do something different in the future (like not throwing an error at all) it's less code to change.

additionally, for downloading native binaries, shouldn't we be using targetPlatfrom instead if hostPlatform? the "cross compilation" logic seems fairly simple in these cases: just download the binaries for the target platform.

@ghost
Copy link
Author

ghost commented Apr 5, 2024

actually, why not factor this off into a lib function? that way if we want to make it do something different in the future (like not throwing an error at all) it's less code to change.

thought about this but i don't think there is too much benefit to a library function other than not having to re-write the message text on every use. the or throw seems standard across nixpkgs and there is not currently a list of common exceptions / errors so adding unsupportedSystemError = system: throw "msg txt ${system}"; would be the first and getting buy in and performing treewide replacement is out of the scope of this change, in my opinion. not sure what the different behavior would be but the throw happens on assignment to various types so if not generating an exception not sure what other correct use case is.

additionally, for downloading native binaries, shouldn't we be using targetPlatfrom instead if hostPlatform? the "cross compilation" logic seems fairly simple in these cases: just download the binaries for the target platform.

hostPlatform is correct. that is where the binaries run.

https://nix.dev/tutorials/cross-compilation.html#platforms

When compiling code, we can distinguish between the build platform, where the executable is built, and the host platform, where the compiled executable runs.

Copy link
Contributor

@lolbinarycat lolbinarycat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving this because i think it's a good short-term solution, although something like a new trivial build helper for prebuilt binaries might be nice to have in the future.

@ghost ghost merged commit 76422a7 into NixOS:master Apr 5, 2024
21 checks passed
@ghost ghost deleted the throw-unsupported-src branch April 5, 2024 21:20
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.

None yet

1 participant