Skip to content

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

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
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

@alexkozy alexkozy commented Aug 29, 2018

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

nodejs-github-bot commented Aug 29, 2018

@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
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

alexkozy commented Aug 30, 2018

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

addaleax commented Aug 31, 2018

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

@addaleax addaleax addaleax approved these changes

@targos targos targos approved these changes

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

@jasnell jasnell Awaiting requested review from jasnell

+1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

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