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

Platform config improvements #105294

Merged
merged 3 commits into from
Dec 2, 2020

Conversation

Ericson2314
Copy link
Member

Motivation for this change

The first commit is about making the selection of a "platform" less haphazard. The rest are about trying to clean up exactly what's there so it's a bit less ad-hoc. (I hope to do more on that front later.)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.


select = platform:
# x86
/**/ if platform.isx86_32 then pc32
Copy link
Member

Choose a reason for hiding this comment

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

these should verify we are still looking at linux systems

Copy link
Member

Choose a reason for hiding this comment

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

Actually the original didn’t check that for pcBase so maybe it shouldn’t matter. I guess defaulting yo pcBase if non linux is reasonable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I would rather decide what to do with that later. If some non-linux platform has some linux config stuff, it should be harmless.

@Ericson2314 Ericson2314 force-pushed the platform-config-improvements branch from 40b8872 to 9918ba2 Compare November 29, 2020 00:04
@Ericson2314
Copy link
Member Author

@matthewbauer OK got all your suggestions.

@Ericson2314 Ericson2314 merged commit 8e21ce5 into NixOS:master Dec 2, 2020
@Ericson2314 Ericson2314 deleted the platform-config-improvements branch December 2, 2020 16:17
@matthewbauer
Copy link
Member

Sorry - forgot to approve - but looks good!

@@ -187,6 +187,7 @@ stdenv.mkDerivation {
else if targetPlatform.isAlpha then "alpha"
else if targetPlatform.isVc4 then "vc4"
else if targetPlatform.isOr1k then "or1k"
else if targetPlatform.isRiscV then "lriscv"
Copy link
Member

Choose a reason for hiding this comment

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

Did you test this? I tried to build NixOS on RISC-V a few months ago and I added "riscv" which seemed to work.
I'm now rebasing my branch on top of master and I have a conflict here.

@r-burns r-burns mentioned this pull request Feb 18, 2021
name = "riscv-multiplatform";
kernelArch = "riscv";
bfdEmulation = "elf${bits}lriscv";
Copy link
Member Author

Choose a reason for hiding this comment

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

@JohnAZoidberg it's based off of this which has the "l", presumably for little endian.

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.

4 participants