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: remove .git folders when testing V8 #32877

Closed
wants to merge 1 commit into from

Conversation

richardlau
Copy link
Member

When running make test-v8 V8's gclient sync converts folders
under deps/v8/third_party into git repositories. Unfortunately
the files that were checked in under deps/v8/third_party/zlib
have been modified from the upstream Chromium repository (some
files have been deleted and there are whitespace differences in
some of the files that were kept) so whenever the Node.js source
tree is hard reset/checked out gclient sync notices there are
unstaged changes as the files in the Node.js source tree do not
match those of the upstream Chromium third party zlib commit.

Refs: nodejs/build#2256

cc @nodejs/v8-update @mmarchini

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

When running `make test-v8` V8's `gclient sync` converts folders
under `deps/v8/third_party` into git repositories. Unfortunately
the files that were checked in under `deps/v8/third_party/zlib`
have been modified from the upstream Chromium repository (some
files have been deleted and there are whitespace differences in
some of the files that were kept) so whenever the Node.js source
tree is hard reset/checked out `gclient sync` notices there are
unstaged changes as the files in the Node.js source tree do not
match those of the upstream Chromium third party zlib commit.

Signed-off-by: Richard Lau <[email protected]>
@nodejs-github-bot nodejs-github-bot added tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Apr 16, 2020
@richardlau
Copy link
Member Author

So I'm not entirely comfortable with this fix but it's probably the quickest way to unbreak the V8 test jobs.

Since V8's build checks out third_party and there is a discrepancy between what we have checked in to our repository in deps/v8/third_party/zlib and the upstream commit

node/deps/v8/DEPS

Lines 264 to 265 in 4a6a5c3

'v8/third_party/zlib':
Var('chromium_url') + '/chromium/src/third_party/zlib.git'+ '@' + 'b9b9a5af7cca2e683e5f2aead8418e5bf9d5a7d5',
it means that the test job is not strictly testing the code we have checked in (because the files get replaced when the V8 build runs gclient sync).

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 16, 2020

@richardlau
Copy link
Member Author

The V8 CI is failing on LinuxONE while compiling V8 tests:
https://ci.nodejs.org/job/node-test-commit-v8-linux/3030/nodes=rhel7-s390x,v8test=v8test/console

15:12:38 ../../test/cctest/parsing/test-scanner-streams.cc: In function ‘void TestUtf8AdvanceUntilOverChunkBoundaries()’:
15:12:38 ../../test/cctest/parsing/test-scanner-streams.cc:334:12: error: ‘char* strncpy(char*, const char*, size_t)’ output truncated copying between 1 and 15 bytes from a string of length 15 [-Werror=stringop-truncation]
15:12:38      strncpy(buffer, unicode_utf8, i);
15:12:38      ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
15:12:38 ../../test/cctest/parsing/test-scanner-streams.cc: In function ‘void TestUtf8ChunkBoundaries()’:
15:12:38 ../../test/cctest/parsing/test-scanner-streams.cc:363:12: error: ‘char* strncpy(char*, const char*, size_t)’ output truncated copying between 1 and 15 bytes from a string of length 15 [-Werror=stringop-truncation]
15:12:38      strncpy(buffer, unicode_utf8, i);
15:12:38      ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
15:12:38 cc1plus: all warnings being treated as errors

I believe the stringop-truncation warning is new to g++ 8 (we recently updated the Centos7/RHEL7 CI hosts to use devtoolset-8 for Node.js 14+) but then we're not seeing it on ppcle (https://ci.nodejs.org/job/node-test-commit-v8-linux/3030/nodes=centos7-ppcle,v8test=v8test/) and both are using g++ (GCC) 8.3.1 20190311 (Red Hat 8.3.1-3).

In any case I don't think it's been caused by this PR and was rather masked due to the V8 job being broken since we landed the V8 8.1 update. I'll defer to @nodejs/v8-update as to whether this failure blocks this PR from landing.

(We don't see this compile failure in the regular CI as we don't build V8 tests there.)

cc FYI @nodejs/platform-s390

@mmarchini
Copy link
Contributor

Build worked for a while after landing 8.1, so it probably broke due to some upstream changes afterwards. But I agree, this was likely masked by the build failures. We can work to fix it after this PR lands.

@mmarchini
Copy link
Contributor

Can we fast-track this to unbreak the CI?

@mmarchini mmarchini added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 16, 2020
@mmarchini
Copy link
Contributor

Landed in 6d77df8

@mmarchini mmarchini closed this Apr 16, 2020
mmarchini pushed a commit that referenced this pull request Apr 16, 2020
When running `make test-v8` V8's `gclient sync` converts folders
under `deps/v8/third_party` into git repositories. Unfortunately
the files that were checked in under `deps/v8/third_party/zlib`
have been modified from the upstream Chromium repository (some
files have been deleted and there are whitespace differences in
some of the files that were kept) so whenever the Node.js source
tree is hard reset/checked out `gclient sync` notices there are
unstaged changes as the files in the Node.js source tree do not
match those of the upstream Chromium third party zlib commit.

Signed-off-by: Richard Lau <[email protected]>

PR-URL: #32877
Refs: nodejs/build#2256
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
mmarchini pushed a commit to mmarchini/node that referenced this pull request Apr 16, 2020
When running `make test-v8` V8's `gclient sync` converts folders
under `deps/v8/third_party` into git repositories. Unfortunately
the files that were checked in under `deps/v8/third_party/zlib`
have been modified from the upstream Chromium repository (some
files have been deleted and there are whitespace differences in
some of the files that were kept) so whenever the Node.js source
tree is hard reset/checked out `gclient sync` notices there are
unstaged changes as the files in the Node.js source tree do not
match those of the upstream Chromium third party zlib commit.

Signed-off-by: Richard Lau <[email protected]>

PR-URL: nodejs#32877
Refs: nodejs/build#2256
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 17, 2020
When running `make test-v8` V8's `gclient sync` converts folders
under `deps/v8/third_party` into git repositories. Unfortunately
the files that were checked in under `deps/v8/third_party/zlib`
have been modified from the upstream Chromium repository (some
files have been deleted and there are whitespace differences in
some of the files that were kept) so whenever the Node.js source
tree is hard reset/checked out `gclient sync` notices there are
unstaged changes as the files in the Node.js source tree do not
match those of the upstream Chromium third party zlib commit.

Signed-off-by: Richard Lau <[email protected]>

PR-URL: #32877
Refs: nodejs/build#2256
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
When running `make test-v8` V8's `gclient sync` converts folders
under `deps/v8/third_party` into git repositories. Unfortunately
the files that were checked in under `deps/v8/third_party/zlib`
have been modified from the upstream Chromium repository (some
files have been deleted and there are whitespace differences in
some of the files that were kept) so whenever the Node.js source
tree is hard reset/checked out `gclient sync` notices there are
unstaged changes as the files in the Node.js source tree do not
match those of the upstream Chromium third party zlib commit.

Signed-off-by: Richard Lau <[email protected]>

PR-URL: nodejs#32877
Refs: nodejs/build#2256
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
When running `make test-v8` V8's `gclient sync` converts folders
under `deps/v8/third_party` into git repositories. Unfortunately
the files that were checked in under `deps/v8/third_party/zlib`
have been modified from the upstream Chromium repository (some
files have been deleted and there are whitespace differences in
some of the files that were kept) so whenever the Node.js source
tree is hard reset/checked out `gclient sync` notices there are
unstaged changes as the files in the Node.js source tree do not
match those of the upstream Chromium third party zlib commit.

Signed-off-by: Richard Lau <[email protected]>

PR-URL: #32877
Refs: nodejs/build#2256
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
targos pushed a commit that referenced this pull request Apr 28, 2020
When running `make test-v8` V8's `gclient sync` converts folders
under `deps/v8/third_party` into git repositories. Unfortunately
the files that were checked in under `deps/v8/third_party/zlib`
have been modified from the upstream Chromium repository (some
files have been deleted and there are whitespace differences in
some of the files that were kept) so whenever the Node.js source
tree is hard reset/checked out `gclient sync` notices there are
unstaged changes as the files in the Node.js source tree do not
match those of the upstream Chromium third party zlib commit.

Signed-off-by: Richard Lau <[email protected]>

PR-URL: #32877
Refs: nodejs/build#2256
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants