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

Revert "http-parser: Broken on Pure Darwin" #17609

Closed
wants to merge 2 commits into from
Closed

Revert "http-parser: Broken on Pure Darwin" #17609

wants to merge 2 commits into from

Conversation

the-kenny
Copy link
Contributor

Motivation for this change

The reverted commit prevents evaluation of http-parser on Darwin because it doesn't work on pure-darwin. While it looks like a good idea to disable this, it prevents the whole node as well as the whole rust infrastructure from evaluating on Darwin, even if people don't use pure-darwin.

Things done
  • Tested using sandboxing
    (nix.useChroot on NixOS,
    or option build-use-chroot in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • 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.

@mention-bot
Copy link

@the-kenny, thanks for your PR! By analyzing the annotation information on this pull request, we identified @dezgeg, @shlevy and @edolstra to be potential reviewers

@the-kenny the-kenny mentioned this pull request Aug 9, 2016
Merged
7 tasks
@the-kenny the-kenny mentioned this pull request Aug 9, 2016
Closed
@dezgeg
Copy link
Contributor

dezgeg commented Aug 9, 2016

Yes, I guess that has to be done in case someone doesn't know how to fix the build to be pure. (Or maybe @copumpkin would know a third way to solve this). Though I think on top on that you should add hydraPlatforms = []; to keep the broken jobs away from Hydra.

@copumpkin
Copy link
Member

Hmm, pure-darwin is a branch name that hasn't been in use in months. Perhaps it actually works nowadays?

@dezgeg
Copy link
Contributor

dezgeg commented Aug 9, 2016

It's not building in Hydra: http://hydra.nixos.org/build/38320273/nixlog/1/raw

@the-kenny
Copy link
Contributor Author

I agree that we should not build it on Hydra. Will add that line tomorrow.

the-kenny referenced this pull request Aug 11, 2016
@dezgeg
http-parser: Broken on Darwin
ea14f5c 
http://hydra.nixos.org/build/38320273
@the-kenny
Copy link
Contributor Author

Anyone having any doubts against merging this? If not I'd go for it.

@retrry
Copy link
Contributor

retrry commented Sep 7, 2016 •
edited

Hey @the-kenny and @dezgeg.
I had some time so I've tried to investigate this issue a bit. What do you think about a way, where we would patch gyp itself on Darwin to not err, when Xcode is not found?
There is a line in nodejs package definition: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/web/nodejs/nodejs.nix#L40
As I understand it patches out raised exception. When I add prePatch line like that to gyp package definition (in python-packages) I can compile http-parse without problems.

I'm hesitant to make a patch like this as I don't seem to grasp all implications of patching gyp on Darwin like that. What are your thoughts? I really would like to get rust building on Darwin again :)

$ nix-build -A http-parser
these derivations will be built:
  /nix/store/a7wp31nh8j7rg56jfd5ccijmcm9lc4gq-http-parser-2.7.1.drv
building path(s) ‘/nix/store/1h260mkn0j9igfzc5pfpnnaz77y0g94i-http-parser-2.7.1’
unpacking sources
unpacking source archive /nix/store/fj8sfbjk795yhbrmdkadd0z7hmgcq87s-v2.7.1.tar.gz
source root is http-parser-2.7.1
setting SOURCE_DATE_EPOCH to timestamp 1468797789 of file http-parser-2.7.1/test.c
patching sources
applying patch /nix/store/x3rg29m48h5lkn200mysqz3kr8vq7rz8-build-shared.patch
patching file http_parser.gyp
configuring
No receipt for 'com.apple.pkg.CLTools_Executables' found at '/'.

No receipt for 'com.apple.pkg.DeveloperToolsCLILeo' found at '/'.

No receipt for 'com.apple.pkg.DeveloperToolsCLI' found at '/'.

building
make flags: SHELL=/nix/store/620nnk750xqfzh6rnmpk4vbsnj7if491-bash-4.3-p46/bin/bash   BUILDTYPE=Release 
  CC(target) out/Release/obj.target/http_parser/http_parser.o
  SOLINK(target) out/Release/libhttp_parser.dylib
  CC(target) out/Release/obj.target/http_parser_strict/http_parser.o
  SOLINK(target) out/Release/libhttp_parser_strict.dylib
  CC(target) out/Release/obj.target/test-nonstrict/test.o
  LINK(target) out/Release/test-nonstrict
  CC(target) out/Release/obj.target/test-strict/test.o
  LINK(target) out/Release/test-strict
installing
post-installation fixup
/nix/store/1h260mkn0j9igfzc5pfpnnaz77y0g94i-http-parser-2.7.1/lib/libhttp_parser.dylib: fixing dylib
/nix/store/1h260mkn0j9igfzc5pfpnnaz77y0g94i-http-parser-2.7.1/lib/libhttp_parser_strict.dylib: fixing dylib
stripping (with flags -S) in /nix/store/1h260mkn0j9igfzc5pfpnnaz77y0g94i-http-parser-2.7.1/lib 
patching script interpreter paths in /nix/store/1h260mkn0j9igfzc5pfpnnaz77y0g94i-http-parser-2.7.1
/nix/store/1h260mkn0j9igfzc5pfpnnaz77y0g94i-http-parser-2.7.1

@kirelagin
Copy link
Member

Is just removing the exception enough? There is also a patch applied (cc @shlevy @svanderburg).

@retrry @the-kenny @copumpkin

@LnL7 LnL7 mentioned this pull request Dec 18, 2016
Merged
6 tasks
@LnL7 LnL7 closed this Dec 18, 2016
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
0.kind: question 6.topic: darwin Running or building packages on Darwin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@the-kenny @mention-bot @dezgeg @copumpkin @retrry @kirelagin @LnL7