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

plan9port: use C compiler from Nix #66404

Merged

Conversation

eraserhd
Copy link
Contributor

@eraserhd eraserhd commented Aug 9, 2019

The install script was escaping the Nix environment on Mac OS by using xcrun -sdk macos clang as its C compiler. Using the Nix compiler required declaring the necessary frameworks as inputs and patching build scripts to assume MacOS 10.12 (and not try to detect).

So cached derivations prior to this would probably not work on all intended target machines.

This should also fix installCheck on Darwin on Hydra.

Other minor fixes:

  • Disable parallel building due to a race with a missing y.tab.h
  • Use NIX_CFLAGS_COMPILE/NIX_LDFLAGS instead of trying to synthesize something like them.
  • X11 dependencies aren't used on Darwin when the windowing system is correctly detected
Motivation for this change

Making the Mac plan9port pure. On Darwin, the package now builds with correct clang 7
instead of Apple's clang 10.

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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
System Before After Delta
Mac OS 215,153,296 885,944,392 +670,791,096
NixOS 454,124,224 488,735,616 +34,611,392
Notify maintainers

cc @AndersonTorres @bbarker @ftrvxmtrx @KoviRobi (and @alyssais)

The install script was escaping the Nix environment on Mac OS by using
`xcrun -sdk macos clang` as its C compiler.  Using the Nix compiler
required declaring the necessary frameworks as inputs and patching
build scripts to assume MacOS 10.12 (and not try to detect).

So cached derivations prior to this would probably not work on all
intended target machines.

This *might* also fix installCheck on Darwin on Hydra.

Other minor fixes:

* Disable parallel building due to a race with a missing y.tab.h
* Use NIX_CFLAGS_COMPILE/NIX_LDFLAGS instead of trying to synthesize
  something like them.
* X11 dependencies aren't used on Darwin when the windowing system
  is correctly detected
@alyssais
Copy link
Member

alyssais commented Aug 12, 2019 via email

@eraserhd
Copy link
Contributor Author

@alyssais yes, but no response ( 9fans/plan9port#272 and 9fans/plan9port#273 )

@eraserhd
Copy link
Contributor Author

Another week, still no response from upstream. Any way to get this merged?

@AndersonTorres
Copy link
Member

AndersonTorres commented Aug 25, 2019

Particularly I don't think we need to wait upstream to incorporate our patches. We can maintain them here and now. If the upstream accepts them, better!

Edit: If in seven days no one objects, I will merge it.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/58

@eraserhd
Copy link
Contributor Author

eraserhd commented Sep 9, 2019

@AndersonTorres it's been 15 days now, but also I just added a small adjustment so that packages that use 9l/9c don't need to have which in the path.

@ofborg ofborg bot requested a review from KoviRobi September 9, 2019 16:49
@AndersonTorres
Copy link
Member

@eraserhd thanks! I will update it now!

@AndersonTorres AndersonTorres merged commit 868bf10 into NixOS:master Sep 10, 2019
@ylh ylh mentioned this pull request Jul 30, 2022
13 tasks
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

4 participants