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

deps: fix CRLF in text file not present in upstream #22340

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Aug 15, 2018

deps/v8/third_party/jinja2/LICENSE is not in upstream v8,
and contains CRLF, which is in conflict with deps/v8/.gitattributes
which sets all text files to use LF.
This has caused failures in CI workers with older versions of Git.
This patch manually fixes up the file to use LF to resolve
the conflict.

The file has already been fixed in upstream jinja2,
which is pull into our repo when we update V8 so it should
be fixed the next time we update V8.

Refs: nodejs/build#1443
Refs: nodejs/reliability#12
Refs: nodejs/build#1453
Refs: https://chromium-review.googlesource.com/c/993812/

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

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Aug 15, 2018
@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 15, 2018

@joyeecheung
Copy link
Member Author

@targos @nodejs/v8-update Do you know where deps/v8/third_party/jinja2/LICENSE is coming from? We may need to update whatever script that fetch it into our repo so that CRLF will be replaced by LF on checkout

@joyeecheung
Copy link
Member Author

Failures in job https://ci.nodejs.org/job/node-test-commit/20564/

ubuntu1804-docker

See failures on test-digitalocean-ubuntu1804_container-x64-1:
ERROR: Step ?Record fingerprints of files to track usage? failed: no workspace for node-test-commit-linux/nodes=ubuntu1804-docker #20760
Notifying upstream projects of job completion
[EnvInject] - [ERROR] - SEVERE ERROR occurs: Channel "unknown": Remote call on JNLP4-connect connection from 128.199.198.56/128.199.198.56:57812 failed. The channel is closing down or has closed down
Agent went offline during the build
Build step 'Trigger parameterized build on other projects' marked build as failure
Finished: FAILURE

ubuntu1604_sharedlibs_openssl110_x64

See failures on test-softlayer-ubuntu1604_sharedlibs_container-x64-5:
not ok 319 parallel/test-crypto-scrypt
  ---
  duration_ms: 1.664
  severity: fail
  exitcode: 1
  stack: |-
    /home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_openssl110_x64/test/common/index.js:735
            throw new assert.AssertionError({
            ^
    
    AssertionError [ERR_ASSERTION]: Expected inputs to be strictly equal:
    + actual - expected ... Lines skipped
    
      Comparison {
    +   message: 'error:060B50AC:digital envelope routines:EVP_PBE_scrypt:memory limit exceeded',
    -   code: 'ERR_CRYPTO_SCRYPT_INVALID_PARAMETER',
    -   message: 'Invalid scrypt parameter',
        type: [Function: Error] {
    ...

ubuntu1604_sharedlibs_openssl110_x64

See failures on test-softlayer-ubuntu1604_sharedlibs_container-x64-5:
not ok 1742 parallel/test-tls-passphrase
  ---
  duration_ms: 0.449
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:650
        throw actual;
        ^
    
    Error: error:06065064:digital envelope routines:EVP_DecryptFinal_ex:bad decrypt
        at Object.createSecureContext (_tls_common.js:144:17)
        at Object.connect (_tls_wrap.js:1136:48)
        at /home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_openssl110_x64/test/parallel/test-tls-passphrase.js:228:7
        at getActual (assert.js:563:5)
        at Function.throws (assert.js:680:24)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_openssl110_x64/test/parallel/test-tls-passphrase.js:227:8)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
  ...

Resume build: https://ci.nodejs.org/job/node-test-commit/20565/

@refack
Copy link
Contributor

refack commented Aug 15, 2018

I would like to wait for @targos RE updating

'v8_embedder_string': '-node.14',

@richardlau
Copy link
Member

Do you know where deps/v8/third_party/jinja2/LICENSE is coming from? We may need to update whatever script that fetch it into our repo so that CRLF will be replaced by LF on checkout

@joyeecheung https://github.com/nodejs/node-core-utils/tree/master/lib/update-v8 (referenced from https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md#major-updates)?

@joyeecheung
Copy link
Member Author

@richardlau Thanks, looks like this has already been fixed in the upstream jinja2

https://chromium.googlesource.com/chromium/src/third_party/jinja2.git/+/b41863e42637544c2941b574c7877d3e1f663e25

@joyeecheung
Copy link
Member Author

We don't have special workflows for third party deps of V8, so we should probably just fix it with the commit message mentioning the upstream patch. The next time V8 is updated it will be overwritten with the upstream fix.

@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 15, 2018

Only two known failures from the resumed build (fix in #22318):

Failures in job https://ci.nodejs.org/job/node-test-commit/20565/

ubuntu1604_sharedlibs_openssl110_x64

See failures on test-softlayer-ubuntu1604_sharedlibs_container-x64-5:
not ok 317 parallel/test-crypto-scrypt
  ---
  duration_ms: 0.737
  severity: fail
  exitcode: 1
  stack: |-
    /home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_openssl110_x64/test/common/index.js:735
            throw new assert.AssertionError({
            ^
    
    AssertionError [ERR_ASSERTION]: Expected inputs to be strictly equal:
    + actual - expected ... Lines skipped
    
      Comparison {
    +   message: 'error:060B50AC:digital envelope routines:EVP_PBE_scrypt:memory limit exceeded',
    -   code: 'ERR_CRYPTO_SCRYPT_INVALID_PARAMETER',
    -   message: 'Invalid scrypt parameter',
        type: [Function: Error] {
    ...

ubuntu1604_sharedlibs_openssl110_x64

See failures on test-softlayer-ubuntu1604_sharedlibs_container-x64-5:
not ok 1741 parallel/test-tls-passphrase
  ---
  duration_ms: 0.282
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:650
        throw actual;
        ^
    
    Error: error:06065064:digital envelope routines:EVP_DecryptFinal_ex:bad decrypt
        at Object.createSecureContext (_tls_common.js:144:17)
        at Object.connect (_tls_wrap.js:1136:48)
        at /home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_openssl110_x64/test/parallel/test-tls-passphrase.js:228:7
        at getActual (assert.js:563:5)
        at Function.throws (assert.js:680:24)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_openssl110_x64/test/parallel/test-tls-passphrase.js:227:8)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
  ...

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 15, 2018
@refack
Copy link
Contributor

refack commented Aug 15, 2018

Failures in job ci.nodejs.org/job/node-test-commit/20565

ubuntu1604_sharedlibs_openssl110_x64
See failures on test-softlayer-ubuntu1604_sharedlibs_container-x64-5:
ubuntu1604_sharedlibs_openssl110_x64
See failures on test-softlayer-ubuntu1604_sharedlibs_container-x64-5:

So essentially master will CI red until #22318 lands?

@Trott
Copy link
Member

Trott commented Aug 15, 2018

So essentially master will CI red until #22318 lands?

That or we temporarily remove the sharedlibs host from the task. Should we do that? Or propose fast-tracking for #22318? #22318 is part of the release going out today and has landed in the v10.9.0-proposal branch: 1ce38878ce. So I imagine it's landing on master is imminent. Should we add a fast-track label to it and suggest people approve it for fast-tracking?

@refack
Copy link
Contributor

refack commented Aug 15, 2018

Moving discussion to the more relevant nodejs/build#1451

@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 16, 2018

@joyeecheung
Copy link
Member Author

The CI is green now

@joyeecheung joyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 16, 2018
@joyeecheung
Copy link
Member Author

I put the fast-track label on this PR. Please thumbs up if you think we can fast track this.

Ping @targos do we need to update the embedder suffix for an update to deps/v8/third_party/jinja2/LICENSE? Personally I don't think so because this patch does not change the source (nor the docs in any meaningful way), and the file touched does not come from V8 - it comes from jinja2 which is a 3rd-party dependency of V8.

@richardlau
Copy link
Member

@refack
Copy link
Contributor

refack commented Aug 17, 2018

I don't think so because this patch does not change the source

I think there are tooling that match number of commits to the v8_embedder_string.
Ref: #22017 (comment)
Let's just update it, and land this...

@jasnell
Copy link
Member

jasnell commented Aug 17, 2018

I'm fine with that too as long as it means we can get this landed.

`deps/v8/third_party/jinja2/LICENSE` is not in upstream v8,
and contains CRLF, which is in conflict with `deps/v8.gitattributes`
which sets all text files to use LF.
This has caused failures in CI workers with older versions of Git.
This patch manually fixes up the file to use LF to resolve
the conflict.

The file has already been fixed in upstream jinja2,
which is pull into our repo when we update V8 so it should
be fixed the next time we update V8.

PR-URL: nodejs#22340
Refs: nodejs/build#1443
Refs: nodejs/reliability#12
Refs: nodejs/build#1453
Refs: https://chromium-review.googlesource.com/c/993812/
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@refack
Copy link
Contributor

refack commented Aug 17, 2018

@refack
Copy link
Contributor

refack commented Aug 17, 2018

@Trott
Copy link
Member

Trott commented Aug 17, 2018

(CI is very red....)

@joyeecheung
Copy link
Member Author

The node-test-pull-request job always rebases so it will fail.

node-test-commit: https://ci.nodejs.org/job/node-test-commit/20631/

@refack
Copy link
Contributor

refack commented Aug 17, 2018

The node-test-pull-request job always rebases so it will fail.

Well that's a bug.
P.S. Job 16508 failed because of a network failure.

@joyeecheung
Copy link
Member Author

Failures in job https://ci.nodejs.org/job/node-test-commit/20631/

freebsd11-x64

See failures on test-digitalocean-freebsd11-x64-2:
bash: line 2: syntax error near unexpected token `newline'

rhel72-s390x

See failures on test-linuxonecc-rhel72-s390x-3:
Build timed out (after 3 minutes). Marking the build as failed.

COMPILED_BY=vs2017,RUNNER=win10,RUN_SUBSET=1

See failures on test-azure_msft-win10-x64-3:
ERROR: Error fetching remote repo 'jenkins_tmp'
hudson.plugins.git.GitException: Failed to fetch from [email protected]:binary_tmp.git
	at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:889)
	at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1146)
	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1177)
	at hudson.scm.SCM.checkout(SCM.java:504)

Resume build: https://ci.nodejs.org/job/node-test-commit/20635/

@joyeecheung
Copy link
Member Author

Windows build cannot be resumed, so started a new one: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20023/ (but I suspect it may still fail due to nodejs/build#1460)

@joyeecheung
Copy link
Member Author

The only error is nodejs/build#1460

@joyeecheung
Copy link
Member Author

Landed in 478a78b

joyeecheung added a commit that referenced this pull request Aug 17, 2018
`deps/v8/third_party/jinja2/LICENSE` is not in upstream v8,
and contains CRLF, which is in conflict with `deps/v8/.gitattributes`
which sets all text files to use LF.
This has caused failures in CI workers with older versions of Git.
This patch manually fixes up the file to use LF to resolve
the conflict.

The file has already been fixed in upstream jinja2,
which is pull into our repo when we update V8 so it should
be fixed the next time we update V8.

PR-URL: #22340
Refs: nodejs/build#1443
Refs: nodejs/reliability#12
Refs: nodejs/build#1453
Refs: https://chromium-review.googlesource.com/c/993812/
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 18, 2018

@nodejs/collaborators If you see

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   deps/v8/third_party/jinja2/LICENSE

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	env.properties

no changes added to commit

In the CI again, please try rebasing against commit later than 478a78b (or just the current master) and push to your PR branch before triggering anothe rCI. The CI may not be able to rebase properly if you use the node-test-pull-request job.

targos pushed a commit that referenced this pull request Aug 19, 2018
`deps/v8/third_party/jinja2/LICENSE` is not in upstream v8,
and contains CRLF, which is in conflict with `deps/v8/.gitattributes`
which sets all text files to use LF.
This has caused failures in CI workers with older versions of Git.
This patch manually fixes up the file to use LF to resolve
the conflict.

The file has already been fixed in upstream jinja2,
which is pull into our repo when we update V8 so it should
be fixed the next time we update V8.

PR-URL: #22340
Refs: nodejs/build#1443
Refs: nodejs/reliability#12
Refs: nodejs/build#1453
Refs: https://chromium-review.googlesource.com/c/993812/
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
`deps/v8/third_party/jinja2/LICENSE` is not in upstream v8,
and contains CRLF, which is in conflict with `deps/v8/.gitattributes`
which sets all text files to use LF.
This has caused failures in CI workers with older versions of Git.
This patch manually fixes up the file to use LF to resolve
the conflict.

The file has already been fixed in upstream jinja2,
which is pull into our repo when we update V8 so it should
be fixed the next time we update V8.

PR-URL: #22340
Refs: nodejs/build#1443
Refs: nodejs/reliability#12
Refs: nodejs/build#1453
Refs: https://chromium-review.googlesource.com/c/993812/
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.