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

tests: add some basic stdenv/cc-wrapper tests #28943

Merged
merged 2 commits into from Sep 10, 2017
Merged

Conversation

LnL7
Copy link
Member

@LnL7 LnL7 commented Sep 3, 2017

Motivation for this change
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@dtzWill
Copy link
Member

dtzWill commented Sep 5, 2017

Haven't tried it yet, but looking it over I think it looks great, and is exactly what I was hoping for/suggesting in #28247. Thank you for putting this together.

One thought: would it make sense to split compilation testing from executing the results? Not sure how that might work, my concern was regarding cross-compilers.

Short of a good answer to that, perhaps we can add an assert or something to indicate this is only intended for native compilers?

Anyway I'd rather see this merged without such consideration than stuck trying to accommodate them-- we really should have basic toolchain testing before marking things good for a channel update.

@LnL7
Copy link
Member Author

LnL7 commented Sep 5, 2017

Yeah, to support cross builds this would need some more work and it's probably a bit more tricky to get that working properly on hydra since it requires multiple platforms to test.

@Ericson2314
Copy link
Member

Ericson2314 commented Sep 5, 2017

I think what @dtzWill is saying is, for now, build the test exes but not run them for testing cross compilation. That strikes me as a fine plan.

@LnL7 LnL7 force-pushed the stdenv-tests branch 2 times, most recently from 7c196c4 to 75ff5fe Compare September 10, 2017 20:48
@LnL7 LnL7 merged commit 33c99ab into NixOS:master Sep 10, 2017
@LnL7 LnL7 deleted the stdenv-tests branch September 10, 2017 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants