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

commented Sep 26, 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.

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

@richardlau

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Makefile Show resolved Hide resolved
@rvagg

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

@richardlau

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2019

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 label Sep 26, 2019
@BridgeAR

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

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

Please +1 to allow fast-tracking.

@rvagg
rvagg approved these changes Sep 27, 2019
Trott added 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

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

Landed in f21818e

@Trott Trott closed this Sep 28, 2019
@mhart

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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.

@rvagg

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

@nodejs/releasers any chance of a release soon? Even if it just includes this? We're skipping unofficial-builds for 12.11.0 and docker-node is currently skipping 12.11.0 too (may be working around it in partial form at least). A 12.11.1 would be nice to take the pressure off having to ship a 12.11.0 in these places.

targos added a commit that referenced this pull request Oct 1, 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>
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
@targos targos referenced this pull request Oct 1, 2019
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
Projects
None yet
8 participants
You can’t perform that action at this time.