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

tools: backport tools/install.py for headers #4149

Closed
wants to merge 1 commit into from

Conversation

richardlau
Copy link
Member

Backport the tools/install.py changes from 628a3ab that were missed
when 6fb0b92 backported the corresponding changes to the Makefile to
build the headers only archive.

Fixes #4140

@mscdex mscdex added the tools Issues and PRs related to the tools directory. label Dec 4, 2015
@mscdex
Copy link
Contributor

mscdex commented Dec 4, 2015

Maybe it would be better to target tools: instead of build:?

@rvagg
Copy link
Member

rvagg commented Dec 4, 2015

lgtm, thanks @richardlau

I have no opinion on tools: vs build:

@richardlau
Copy link
Member Author

I've tested by running HEADERS_ONLY=1 python tools/install.py install 'node-v0.12.10' '/' and inspecting the contents of the created node-v0.12.10 directory.

I tried to run make tar-headers but ran into some problems:

  1. First I see this, which is easily patched around
#NODE_VERSION_IS_RELEASE is set to 0.
Did you remember to update src/node_version.h?
  1. If I patch src/node_version.h for 1) I then get:
find node-v0.12.10/ -type l | xargs rm # annoying on windows
rm: missing operand
Try `rm --help' for more information.
make: *** [node-v0.12.10.tar-headers] Error 123

But I also get the same problem with v4.x.

@richardlau
Copy link
Member Author

@mscdex I can retarget to tools: if required -- I picked build: because the two commits referenced were targeted at build:.

@richardlau
Copy link
Member Author

This also affects v0.10 -- Please could someone add a lts-watch-v0.10 label?

Backport the tools/install.py changes from 628a3ab that were missed
when 6fb0b92 backported the corresponding changes to the Makefile to
build the headers only archive.
@richardlau
Copy link
Member Author

Retargetted to tools:.

@rvagg
Copy link
Member

rvagg commented Dec 4, 2015

@richardlau are you on Linux? the xargs thing works on OS X and that's what's used to generate the tarball. I've tried out your branch and it works fine on OS X but not on Linux, that's ... how it is and it kind of sucks but nobody has got around to fixing it yet. There's two instances of the same pattern in there.

@richardlau
Copy link
Member Author

@rvagg Yes I'm on Linux -- Didn't realize that the tarball was generated on OS X. Thanks.

@rvagg
Copy link
Member

rvagg commented Dec 4, 2015

still lgtm, needs to sit for additional reviews and possible objection for a couple of days but someone can land it after that (I'll do it if I remember but any other collaborator can)

@bnoordhuis
Copy link
Member

Untested but LGTM.

@rvagg
Copy link
Member

rvagg commented Dec 4, 2015

sorry, I should have posted results of my testing on 0.12:

$ make tar-headers
/usr/local/opt/python/bin/python2.7 ./configure \
        --prefix=/ \
        --dest-cpu=x64 \
        --tag= \
         --download=all --with-intl=small-icu
creating  ./icu_config.gypi

... configure blah blah ...

creating  ./config.gypi
creating  ./config.mk
HEADERS_ONLY=1 /usr/local/opt/python/bin/python2.7 tools/install.py install 'node-v0.12.10' '/'

.. list of files blah blah ..

find node-v0.12.10/ -type l | xargs rm # annoying on windows
tar -cf node-v0.12.10-headers.tar node-v0.12.10
rm -rf node-v0.12.10
gzip -c -f -9 node-v0.12.10-headers.tar > node-v0.12.10-headers.tar.gz
rm node-v0.12.10-headers.tar
$ ls -al node-v0.12.10-headers.tar.gz
-rw-r--r--  1 rvagg  staff  444046  4 Dec 23:07 node-v0.12.10-headers.tar.gz

The full contents of the resulting file is listed here: https://gist.github.com/rvagg/e86237fb9d838ae57a62

@jbergstroem
Copy link
Member

LGTM

@richardlau richardlau changed the title build: backport tools/install.py for headers tools: backport tools/install.py for headers Dec 5, 2015
rvagg pushed a commit that referenced this pull request Dec 7, 2015
Backport the tools/install.py changes from 628a3ab that were missed
when 6fb0b92 backported the corresponding changes to the Makefile to
build the headers only archive.

PR-URL: #4149
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg pushed a commit that referenced this pull request Dec 7, 2015
Backport the tools/install.py changes from 628a3ab that were missed
when 6fb0b92 backported the corresponding changes to the Makefile to
build the headers only archive.

PR-URL: #4149
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg
Copy link
Member

rvagg commented Dec 7, 2015

lgtm

landed in v0.12-staging @ 5f0b675
landed in v0.10-staging @ 28ab7b0

@rvagg rvagg closed this Dec 7, 2015
rvagg pushed a commit that referenced this pull request Feb 8, 2016
Backport the tools/install.py changes from 628a3ab that were missed
when 6fb0b92 backported the corresponding changes to the Makefile to
build the headers only archive.

PR-URL: #4149
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg pushed a commit that referenced this pull request Feb 8, 2016
Backport the tools/install.py changes from 628a3ab that were missed
when 6fb0b92 backported the corresponding changes to the Makefile to
build the headers only archive.

PR-URL: #4149
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Rod Vagg <rod@vagg.org>
jasnell pushed a commit that referenced this pull request Feb 9, 2016
Backport the tools/install.py changes from 628a3ab that were missed
when 6fb0b92 backported the corresponding changes to the Makefile to
build the headers only archive.

PR-URL: #4149
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Rod Vagg <rod@vagg.org>
jasnell pushed a commit that referenced this pull request Feb 9, 2016
Backport the tools/install.py changes from 628a3ab that were missed
when 6fb0b92 backported the corresponding changes to the Makefile to
build the headers only archive.

PR-URL: #4149
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Rod Vagg <rod@vagg.org>
@gibfahn gibfahn mentioned this pull request Mar 31, 2016
4 tasks
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
Backport the tools/install.py changes from 628a3ab that were missed
when 6fb0b92 backported the corresponding changes to the Makefile to
build the headers only archive.

PR-URL: nodejs/node#4149
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Rod Vagg <rod@vagg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants