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

build: fix test-v8 target #17269

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@targos
Member

targos commented Nov 23, 2017

Clean the deps/v8 directory before rebuilding node for the hash seed
test. It is necessary to avoid the script added in 1 to use ignored
files while generating node-debug-support.cc.

Fixes: nodejs/node-v8#26

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

/cc @bnoordhuis @mmarchini

build: fix test-v8 target
Clean the deps/v8 directory before rebuilding node for the hash seed
test. It is necessary to avoid the script added in [1] to use ignored
files while generating `node-debug-support.cc`.

[1]: #14901

Fixes: nodejs/node-v8#26

@targos targos added the V8 Engine label Nov 23, 2017

@targos targos referenced this pull request Nov 23, 2017

Closed

deps: update V8 to 6.3 #16271

2 of 2 tasks complete
@targos

This comment has been minimized.

Show comment
Hide comment
@targos
Member

targos commented Nov 23, 2017

@@ -501,6 +501,7 @@ test-v8: v8
--no-presubmit \
--shell-dir=$(PWD)/deps/v8/out/$(V8_ARCH).$(BUILDTYPE_LOWER) \
$(TAP_V8)
git clean -fdxq -- deps/v8

This comment has been minimized.

@richardlau

richardlau Nov 23, 2017

Member

Can we assume the presence of git, i.e. is this target valid for running from the source tarball?

@richardlau

richardlau Nov 23, 2017

Member

Can we assume the presence of git, i.e. is this target valid for running from the source tarball?

This comment has been minimized.

@targos

targos Nov 23, 2017

Member

This target depends on the v8 target, which executes make-v8.sh, which already assumes the presence of git.

@targos

targos Nov 23, 2017

Member

This target depends on the v8 target, which executes make-v8.sh, which already assumes the presence of git.

This comment has been minimized.

@refack

refack Nov 23, 2017

Member

But maybe replace x with X?
If I'm understanding git's logic, it'll use node's .git not deps/v8/.git

@refack

refack Nov 23, 2017

Member

But maybe replace x with X?
If I'm understanding git's logic, it'll use node's .git not deps/v8/.git

This comment has been minimized.

@targos

targos Nov 23, 2017

Member

deps/v8/.git is removed in make-v8.sh during cleanup:

find v8 -name ".git" | xargs rm -rf || true

@targos

targos Nov 23, 2017

Member

deps/v8/.git is removed in make-v8.sh during cleanup:

find v8 -name ".git" | xargs rm -rf || true

This comment has been minimized.

@refack

refack Nov 23, 2017

Member

So definatly -X (it will remove explicitly ignored files, while keeping newly added files)

@refack

refack Nov 23, 2017

Member

So definatly -X (it will remove explicitly ignored files, while keeping newly added files)

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Nov 23, 2017

Member

Test failure is unrelated. See #17270

Member

targos commented Nov 23, 2017

Test failure is unrelated. See #17270

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Nov 23, 2017

Member

I want to propose reverting #14901 for now. tools/gen-postmortem-metadata.py needs some more work, IMO. I'll open a PR.

Member

bnoordhuis commented Nov 23, 2017

I want to propose reverting #14901 for now. tools/gen-postmortem-metadata.py needs some more work, IMO. I'll open a PR.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Nov 23, 2017

Member

I see no harm in this change even after 088bba3.
AFAICT make test-hash-seed rebuilds V8 anyway, so there should be no performance lost.

Member

refack commented Nov 23, 2017

I see no harm in this change even after 088bba3.
AFAICT make test-hash-seed rebuilds V8 anyway, so there should be no performance lost.

@refack

refack approved these changes Nov 23, 2017

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 29, 2017

Member

landed in 6c47033

Member

MylesBorins commented Nov 29, 2017

landed in 6c47033

MylesBorins added a commit that referenced this pull request Nov 29, 2017

build: fix test-v8 target
Clean the deps/v8 directory before rebuilding node for the hash seed
test. It is necessary to avoid the script added in [1] to use ignored
files while generating `node-debug-support.cc`.

[1]: #14901

PR-URL: #17269
Fixes: nodejs/node-v8#26
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins added a commit that referenced this pull request Dec 12, 2017

build: fix test-v8 target
Clean the deps/v8 directory before rebuilding node for the hash seed
test. It is necessary to avoid the script added in [1] to use ignored
files while generating `node-debug-support.cc`.

[1]: #14901

PR-URL: #17269
Fixes: nodejs/node-v8#26
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins added a commit that referenced this pull request Dec 12, 2017

build: fix test-v8 target
Clean the deps/v8 directory before rebuilding node for the hash seed
test. It is necessary to avoid the script added in [1] to use ignored
files while generating `node-debug-support.cc`.

[1]: #14901

PR-URL: #17269
Fixes: nodejs/node-v8#26
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@MylesBorins MylesBorins referenced this pull request Dec 12, 2017

Merged

v9.3.0 proposal #17631

gibfahn added a commit that referenced this pull request Dec 19, 2017

build: fix test-v8 target
Clean the deps/v8 directory before rebuilding node for the hash seed
test. It is necessary to avoid the script added in [1] to use ignored
files while generating `node-debug-support.cc`.

[1]: #14901

PR-URL: #17269
Fixes: nodejs/node-v8#26
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

gibfahn added a commit that referenced this pull request Dec 19, 2017

build: fix test-v8 target
Clean the deps/v8 directory before rebuilding node for the hash seed
test. It is necessary to avoid the script added in [1] to use ignored
files while generating `node-debug-support.cc`.

[1]: #14901

PR-URL: #17269
Fixes: nodejs/node-v8#26
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@gibfahn gibfahn referenced this pull request Dec 20, 2017

Closed

v8.9.4 proposal #17772

gibfahn added a commit that referenced this pull request Dec 20, 2017

build: fix test-v8 target
Clean the deps/v8 directory before rebuilding node for the hash seed
test. It is necessary to avoid the script added in [1] to use ignored
files while generating `node-debug-support.cc`.

[1]: #14901

PR-URL: #17269
Fixes: nodejs/node-v8#26
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@gibfahn gibfahn referenced this pull request Dec 20, 2017

Merged

v8.9.4 proposal #17774

@MylesBorins MylesBorins referenced this pull request Dec 20, 2017

Merged

v6.12.3 proposal #17776

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

build: fix test-v8 target
Clean the deps/v8 directory before rebuilding node for the hash seed
test. It is necessary to avoid the script added in [1] to use ignored
files while generating `node-debug-support.cc`.

[1]: nodejs#14901

PR-URL: nodejs#17269
Fixes: nodejs/node-v8#26
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

build: fix test-v8 target
Clean the deps/v8 directory before rebuilding node for the hash seed
test. It is necessary to avoid the script added in [1] to use ignored
files while generating `node-debug-support.cc`.

[1]: nodejs#14901

PR-URL: nodejs#17269
Fixes: nodejs/node-v8#26
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment