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.

Sign up for GitHub

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

Jump to bottom

sl: use appropriate compiler #60023

Merged
merged 1 commit into from Apr 22, 2019
Merged

sl: use appropriate compiler #60023

merged 1 commit into from Apr 22, 2019

Conversation

mnacamura
Copy link
Contributor

Motivation for this change

Current sl cannot be built on Darwin.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@symphorien
Copy link
Member

Maybe it is simpler to use $(CC) instead of gcc, clang or something else.

@grahamc
Copy link
Member

grahamc commented Apr 22, 2019

@GrahamcOfBorg eval

@mnacamura
Copy link
Contributor Author

@symphorien Thanks for your suggestion. Added a make flag instead of patching Makefile.

@infinisil infinisil changed the title sl: use clang if stdenv.cc.isClang sl: use appropriate compiler Apr 22, 2019
@infinisil infinisil merged commit 496d945 into NixOS:master Apr 22, 2019
@infinisil
Copy link
Member

Ah, this was broken in #59109 (ping @peterhoeg)

I guess the previous fix was a bit simpler (buildFlags = [ "CC=cc" ];) but since I already merged it let's leave it at that.

@mnacamura mnacamura deleted the sl branch April 23, 2019 00:39
@peterhoeg
Copy link
Member

Apologies for breaking it - it was fine on NixOS.

Maybe we should either add a comment or do

makeFlags = [] ++ lib.optional stdenv.isDarwin "CC:=$(CC)";

?

This way we make it clear to the next person who comes along.

@infinisil
Copy link
Member

That sounds like a good idea yeah

@symphorien
Copy link
Member

Making this unconditional might potentially also fix cross-compilation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mnacamura @symphorien @grahamc @infinisil @peterhoeg @GrahamcOfBorg