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

build: include deps/v8/test/torque in source tarball #29712

Closed
wants to merge 2 commits into from

Conversation

richardlau
Copy link
Member

Builds from the source tarball were broken by the recent V8 upate
to 7.7 as a file needed to build torque wasn't included in the source
tarball as it resides in deps/v8/test.

Refs: #28918
Fixes: #29709

cc @nodejs/build-files @nodejs/v8-update

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

Builds from the source tarball were broken by the recent V8 upate
to 7.7 as a file needed to build torque wasn't included in the source
tarball as it resides in deps/v8/test.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Sep 26, 2019
@richardlau
Copy link
Member Author

richardlau commented Sep 26, 2019

See also #25097. I first tried to just remove

"<(V8_ROOT)/test/torque/test-torque.tq",
but doing that resulted in these errors:

../../deps/v8/src/builtins/array.tq:35:3: Lint error: Macro 'IsJSArray' is never used.
../../deps/v8/src/builtins/base.tq:462:1: Lint error: Macro 'NewJSArray' is never used.
../../deps/v8/src/builtins/base.tq:3121:1: Lint error: Macro 'VerifiedUnreachable' is never used.
../../deps/v8/src/builtins/base.tq:446:3: Lint error: Macro 'IsEmpty' is never used.
/bin/sh: line 1: 41272 Aborted                 (core dumped)

@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member Author

The regular CI doesn't build the source tarball. I tested this locally on Linux (running make tar after removing the release-only checks). If I remember correctly the release CI doesn't build the source tarball on Linux (I can't check this as I don't have permissions to view the release CI) so some independent verification that the source tarball is still generated corrrectly on a similar environment as used in the release CI would be appreciated.

@bnoordhuis
Copy link
Member

We don't need to distribute that file because it's only used by V8's cctest target, which we don't build. Only lightly tested but I think this is all we need:

diff --git a/tools/v8_gypfiles/v8.gyp b/tools/v8_gypfiles/v8.gyp
index 00e285ec2c..4bc8817261 100644
--- a/tools/v8_gypfiles/v8.gyp
+++ b/tools/v8_gypfiles/v8.gyp
@@ -82,7 +82,6 @@
       "<(V8_ROOT)/src/builtins/typed-array-subarray.tq",
       "<(V8_ROOT)/src/builtins/typed-array.tq",
       "<(V8_ROOT)/third_party/v8/builtins/array-sort.tq",
-      "<(V8_ROOT)/test/torque/test-torque.tq",
     ],
     'torque_output_root': '<(SHARED_INTERMEDIATE_DIR)/torque-output-root',
     'torque_files_replaced': ['<!@pymod_do_main(ForEachReplace ".tq" "-tq-csa" <@(torque_files))'],

@richardlau
Copy link
Member Author

We don't need to distribute that file because it's only used by V8's cctest target, which we don't build. Only lightly tested but I think this is all we need:

diff --git a/tools/v8_gypfiles/v8.gyp b/tools/v8_gypfiles/v8.gyp
index 00e285ec2c..4bc8817261 100644
--- a/tools/v8_gypfiles/v8.gyp
+++ b/tools/v8_gypfiles/v8.gyp
@@ -82,7 +82,6 @@
       "<(V8_ROOT)/src/builtins/typed-array-subarray.tq",
       "<(V8_ROOT)/src/builtins/typed-array.tq",
       "<(V8_ROOT)/third_party/v8/builtins/array-sort.tq",
-      "<(V8_ROOT)/test/torque/test-torque.tq",
     ],
     'torque_output_root': '<(SHARED_INTERMEDIATE_DIR)/torque-output-root',
     'torque_files_replaced': ['<!@pymod_do_main(ForEachReplace ".tq" "-tq-csa" <@(torque_files))'],

@bnoordhuis Tried that already. See #29712 (comment).

@bnoordhuis
Copy link
Member

bnoordhuis commented Sep 26, 2019

Ah, it fails with lint errors emitted by run-torque...

../../deps/v8/src/builtins/array.tq:35:3: Lint error: Macro 'IsJSArray' is never used.
../../deps/v8/src/builtins/base.tq:462:1: Lint error: Macro 'NewJSArray' is never used.
../../deps/v8/src/builtins/base.tq:3121:1: Lint error: Macro 'VerifiedUnreachable' is never used.
../../deps/v8/src/builtins/base.tq:446:3: Lint error: Macro 'IsEmpty' is never used.

edit: sorry, didn't see your comments. I had this page open for a bit but GH didn't auto-refresh for some reason.

Makefile Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

Makefile Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Sep 26, 2019

This includes test/torque/test-torque.tq in the tarball so I guess it lgtm, I have one outstanding question inline though, not a blocker. I'm off for the day and would be happy to see this merged if it fixes the problem so we can get a quick release out to patch this. I've requested a test build using the latest commit on this branch, it should come out at https://nodejs.org/download/test/ soon, probably named v13.0.0-test20190926b516994392. Keep a look out for it and test the source tarball works.

@rvagg
Copy link
Member

rvagg commented Sep 26, 2019

@richardlau
Copy link
Member Author

Yep, filling up @ https://nodejs.org/download/test/v13.0.0-test20190926b516994392/

I've downloaded https://nodejs.org/download/test/v13.0.0-test20190926b516994392/node-v13.0.0-test20190926b516994392.tar.gz and built it successfully on Linux. So I think this is ready bar another review or so.

cc @nodejs/releasers FYI if another quick release is desired.

@BridgeAR BridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label Sep 26, 2019
@BridgeAR
Copy link
Member

I will try to prepare a patch release with this and #29472

Please +1 to allow fast-tracking.

Trott pushed a commit that referenced this pull request Sep 28, 2019
Builds from the source tarball were broken by the recent V8 upate
to 7.7 as a file needed to build torque wasn't included in the source
tarball as it resides in deps/v8/test.

PR-URL: #29712
Fixes: #29709
Refs: #28918
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@Trott
Copy link
Member

Trott commented Sep 28, 2019

Landed in f21818e

@Trott Trott closed this Sep 28, 2019
@mhart
Copy link
Contributor

mhart commented Sep 28, 2019

What are the chances of getting some tarball builds happening so that any potential tarball issues are unlikely to happen in the future?

@rvagg
Copy link
Member

rvagg commented Sep 29, 2019

@mhart given multiple failures with source tarballs it's probably appropriate we start testing it. Would you mind opening an issue in nodejs/build for it?

What would help most getting this off the ground is some bash to run through a build & test that works for any arbitrary commit--not just releases, or a sensible way to mock a release for the purpose of tarball creation so it can be run on every test run.

targos added a commit that referenced this pull request Oct 1, 2019
Notable changes:

* build:
  * This release fixes a regression that prevented from building Node.js
    using the official source tarball.
    #29712
* deps:
  * Updated small-icu data to support "unit" style in the
    `Intl.NumberFormat` API.
    #29735

PR-URL: #29796
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test-torque.tq failing source tarball builds for v12.11.0 (V8 7.7)
8 participants