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

build: do not copy v8-inspector* headers as part of install #22586

Closed
wants to merge 1 commit into from
Closed

build: do not copy v8-inspector* headers as part of install #22586

wants to merge 1 commit into from

Conversation

alexkozy
Copy link
Member

These headers are exposed from V8 for embedder and should not be
used by native addons.

Fixes #22415

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@alexkozy alexkozy requested a review from addaleax August 29, 2018 16:34
@nodejs-github-bot
Copy link
Collaborator

@ak239 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/724/pipeline

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 29, 2018
@alexkozy alexkozy added the inspector Issues and PRs related to the V8 inspector protocol label Aug 29, 2018
@alexkozy
Copy link
Member Author

alexkozy commented Aug 29, 2018 •
edited
Loading

@addaleax
Anna please take a look. I am not sure that this is enough to exclude headers from node release but I was not able to find any better place where we determine what files to add to release tar archive.

addaleax
addaleax approved these changes Aug 30, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the small nit that current style here (and, afaik, in all our Python files) is lambda a: without an extra space

@refack
Copy link
Contributor

refack commented Aug 30, 2018

@ak239 seems like you found the right spot. The make target uses install.py to build the tree that it will tarball:

node/Makefile

Lines 967 to 983 in 503fd55

.PHONY: $(TARBALL)-headers
$(TARBALL)-headers: release-only
$(PYTHON) ./configure \
--prefix=/ \
--dest-cpu=$(DESTCPU) \
--tag=$(TAG) \
--release-urlbase=$(RELEASE_URLBASE) \
$(CONFIG_FLAGS) $(BUILD_RELEASE_FLAGS)
HEADERS_ONLY=1 $(PYTHON) tools/install.py install '$(TARNAME)' '/'
find $(TARNAME)/ -type l | xargs $(RM)
tar -cf $(TARNAME)-headers.tar $(TARNAME)
$(RM) -r $(TARNAME)
gzip -c -f -9 $(TARNAME)-headers.tar > $(TARNAME)-headers.tar.gz
ifeq ($(XZ), 0)
xz -c -f -$(XZ_COMPRESSION) $(TARNAME)-headers.tar > $(TARNAME)-headers.tar.xz
endif
$(RM) $(TARNAME)-headers.tar

refack
refack reviewed Aug 30, 2018
View reviewed changes
tools/install.py Outdated Show resolved Hide resolved
refack
refack approved these changes Aug 30, 2018
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Preferably with nit fixed, but non-blocking.

@refack
Copy link
Contributor

refack commented Aug 30, 2018

/CC @nodejs/python @nodejs/build-files

@refack
Copy link
Contributor

refack commented Aug 30, 2018

P.S. this might be semver-major since it changes the content of our "SDK" (the headers tarball)

@refack refack added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 30, 2018
@alexkozy
Copy link
Member Author

Thanks for review! I addressed comments and started CI: https://ci.nodejs.org/job/node-test-pull-request/16879/

@refack
Copy link
Contributor

refack commented Aug 30, 2018

Resume CI: https://ci.nodejs.org/job/node-test-commit/21063/

@alexkozy
build: do not copy v8-inspector* headers ar part of install
d8999f2 
These headers are exposed from V8 for embedder and should not be
used by native addons.

Fixes #22415
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 31, 2018
@BridgeAR BridgeAR requested a review from a team August 31, 2018 15:37
targos
targos approved these changes Aug 31, 2018
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@addaleax
Copy link
Member

Landed in 4b47d29

@addaleax addaleax closed this Aug 31, 2018
addaleax pushed a commit that referenced this pull request Aug 31, 2018
@alexkozy @addaleax
build: do not copy v8-inspector* headers ar part of install
4b47d29 
These headers are exposed from V8 for embedder and should not be
used by native addons.

Fixes #22415

PR-URL: #22586
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@targos targos mentioned this pull request Aug 31, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@addaleax addaleax addaleax approved these changes

@refack refack refack approved these changes

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. inspector Issues and PRs related to the V8 inspector protocol semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

inspector: mark v8-inspector.h as NODE_WANT_INTERNALS
6 participants
@alexkozy @nodejs-github-bot @refack @addaleax @targos @BridgeAR