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

doc: remove eu-strip from tarball #20304

Closed
wants to merge 2 commits into from
Closed

doc: remove eu-strip from tarball #20304

wants to merge 2 commits into from

Conversation

jvelezpo
Copy link
Contributor

Remove eu-strip from tarball, this is been removed because of this:
the source is not provided
it adds 105ko of useless files
It's only used in the android and fuchsia GN builds

Fixes: #20280

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

@jvelezpo
doc: remove eu-strip from tarball
9e8f256 
This is been removed because of this:
the source is not provided
it adds 105ko of useless files
It's only used in the android and fuchsia GN builds

Fixes: #20280
@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Apr 25, 2018
bnoordhuis
bnoordhuis approved these changes Apr 25, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Can you add /deps/v8/third_party/eu-strip/ to the top-level .gitignore?

addaleax
addaleax approved these changes Apr 25, 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.

Looks good – we just need to make sure we don’t accidentally re-introduce the file when updating V8, I guess.

@jvelezpo
Copy link
Contributor Author

jvelezpo commented Apr 25, 2018 •
edited
Loading

yes im on it @bnoordhuis

and yes @addaleax the issue refers to that

If we do strip this out we should also update the instructions in https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md and the git node v8 command in https://github.com/nodejs/node-core-utils to stop it coming back the next time V8 is updated.

and i am on it, i am only missing where to change the doc on (https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md)

@addaleax
Copy link
Member

@jvelezpo I guess @bnoordhuis’ suggestion would suffice, nothing more necessary than that. I just hadn’t thought of it. :)

@jvelezpo
doc: remove eu-strip from tarball
cbb233d 
This is been removed because of this:
the source is not provided
it adds 105ko of useless files
It's only used in the android and fuchsia GN builds

Fixes: #20280
@richardlau
Copy link
Member

I just hadn’t thought of it. :)

Neither did I 😆 .

@richardlau
Copy link
Member

The subsystem in the commit message should be deps: rather than doc: but that can be fixed on landing.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2018
@BridgeAR
Copy link
Member

CI https://ci.nodejs.org/job/node-test-pull-request/14503/

trivikr pushed a commit that referenced this pull request Apr 27, 2018
@jvelezpo @trivikr
doc: remove eu-strip from tarball
dc8676c 
This is been removed because of this:
the source is not provided
it adds 105ko of useless files
It's only used in the android and fuchsia GN builds

Fixes: #20280

PR-URL: #20304
Fixes: #20280
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@trivikr
Copy link
Member

trivikr commented Apr 27, 2018 •
edited
Loading

Landed in dc8676c

I forgot to verify that CI had failed on some systems, rerunning:

@trivikr trivikr self-assigned this Apr 27, 2018
@trivikr
Copy link
Member

trivikr commented Apr 28, 2018

The CI builds for node-test-commit-arm-fanned and node-test-commit-linux were successful.
Closing this PR as the commit landed in dc8676c

@trivikr trivikr closed this Apr 28, 2018
targos added a commit to targos/node-core-utils that referenced this pull request May 1, 2018
@targos
git-node v8: remove eu-strip during major upgrades
3693aaf 
Ref: nodejs/node#20304
@targos targos mentioned this pull request May 1, 2018
Merged
joyeecheung pushed a commit to nodejs/node-core-utils that referenced this pull request May 3, 2018
@targos @joyeecheung
git-node v8: remove eu-strip during major upgrades (#243)
80ec407 
* git-node v8: remove eu-strip during major upgrades

Ref: nodejs/node#20304
MylesBorins pushed a commit that referenced this pull request May 4, 2018
@jvelezpo @MylesBorins
doc: remove eu-strip from tarball
9c11a18 
This is been removed because of this:
the source is not provided
it adds 105ko of useless files
It's only used in the android and fuchsia GN builds

Fixes: #20280

PR-URL: #20304
Fixes: #20280
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jvelezpo jvelezpo mentioned this pull request May 4, 2018
Closed
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
Merged
@MylesBorins
Copy link
Contributor

bbackported to 8.x. Please lmk if we should revert

MylesBorins pushed a commit that referenced this pull request Aug 17, 2018
@jvelezpo @MylesBorins
doc: remove eu-strip from tarball
24a105f 
This is been removed because of this:
the source is not provided
it adds 105ko of useless files
It's only used in the android and fuchsia GN builds

Fixes: #20280

PR-URL: #20304
Fixes: #20280
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 17, 2018
Merged
johnfrench3 pushed a commit to johnfrench3/core-utils-node that referenced this pull request Nov 2, 2022
git-node v8: remove eu-strip during major upgrades (#243)
47e2139 
* git-node v8: remove eu-strip during major upgrades

Ref: nodejs/node#20304
renawolford6 added a commit to renawolford6/node-dev-build-core-utils that referenced this pull request Nov 10, 2022
@renawolford6
git-node v8: remove eu-strip during major upgrades (#243)
6389c42 
* git-node v8: remove eu-strip during major upgrades

Ref: nodejs/node#20304
Developerarif2 pushed a commit to Developerarif2/node-core-utils that referenced this pull request Jan 27, 2023
@logictitans
git-node v8: remove eu-strip during major upgrades (#243)
c571119 
* git-node v8: remove eu-strip during major upgrades

Ref: nodejs/node#20304
gerkai added a commit to gerkai/node-core-utils-project-build that referenced this pull request Jan 27, 2023
@gerkai
git-node v8: remove eu-strip during major upgrades (#243)
acc5edb 
* git-node v8: remove eu-strip during major upgrades

Ref: nodejs/node#20304
patrickm68 added a commit to patrickm68/NodeJS-core-utils that referenced this pull request Sep 14, 2023
@patrickm68
git-node v8: remove eu-strip during major upgrades (#243)
8fca69a 
* git-node v8: remove eu-strip during major upgrades

Ref: nodejs/node#20304
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@richardlau richardlau richardlau approved these changes

@mmarchini mmarchini mmarchini approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mithunsasidharan mithunsasidharan mithunsasidharan approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees

@trivikr trivikr

Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.