deps: fix CRLF in text file not present in upstream by joyeecheung · Pull Request #22340 · nodejs/node · GitHub

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

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

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1605/
CI: https://ci.nodejs.org/job/node-test-pull-request/16468/

Not rebasing:
https://ci.nodejs.org/job/node-test-commit/20564/
https://ci.nodejs.org/job/node-test-commit-v8-linux/1606/

@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

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/

I would like to wait for @targos RE updating

node/common.gypi

Line 32 in 8a41470

'v8_embedder_string': '-node.14',

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)?

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

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

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.

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)
  ...

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?

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?

Moving discussion to the more relevant nodejs/build#1451

Rebased after #22318 lands, let's see if CI is happy now

https://ci.nodejs.org/job/node-test-commit/20588/
https://ci.nodejs.org/job/node-test-commit-v8-linux/1610/

The CI is green now

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.

@targos is on holiday until Monday.

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

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 <riclau@uk.ibm.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

CI: https://ci.nodejs.org/job/node-test-pull-request/16507/

New CI (no rebase) https://ci.nodejs.org/job/node-test-pull-request/16508/

(CI is very red....)

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

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

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.

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 binary_tmp@147.75.70.237: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/

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)

The only error is nodejs/build#1460

Landed in 478a78b

`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 <riclau@uk.ibm.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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

`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 <riclau@uk.ibm.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Aug 15, 2018

joyeecheung requested a review from targos August 15, 2018 17:32

richardlau approved these changes Aug 15, 2018

View reviewed changes

joaocgreis approved these changes Aug 15, 2018

View reviewed changes

joyeecheung mentioned this pull request Aug 15, 2018

CI failures: 20180815 nodejs/reliability#13

Closed

mmarchini approved these changes Aug 15, 2018

View reviewed changes

joaocgreis mentioned this pull request Aug 15, 2018

Setting autocrlf and safecrlf on workers nodejs/build#1453

Closed

12 tasks

lpinca approved these changes Aug 15, 2018

View reviewed changes

maclover7 approved these changes Aug 15, 2018

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 15, 2018

joyeecheung force-pushed the use-lf-in-v8 branch from 342f1f3 to 4b2b187 Compare August 16, 2018 12:49

BridgeAR approved these changes Aug 16, 2018

View reviewed changes

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

jasnell approved these changes Aug 17, 2018

View reviewed changes

refack force-pushed the use-lf-in-v8 branch from 4b2b187 to 76c07bf Compare August 17, 2018 00:52

joyeecheung mentioned this pull request Aug 17, 2018

Git lock in test-azure_msft-win10-x64-3 exists nodejs/build#1460

Closed

joyeecheung closed this Aug 17, 2018

This was referenced Aug 17, 2018

CI Failures: 20180817 nodejs/reliability#14

Closed

test-digitalocean-fedora27-x64-1 keeps timing out nodejs/build#1463

Closed

joyeecheung mentioned this pull request Aug 20, 2018

CI failures: 20180820 nodejs/reliability#15

Closed

refack mentioned this pull request Aug 20, 2018

Immediate failures on smartos, linux-containered, arm, and linuxone nodejs/build#1467

Closed

This was referenced Aug 24, 2018

module: accept Windows relative path #22186

Closed

child-process: support any shells on Windows #21943

Closed

refack mentioned this pull request Sep 4, 2018

Node.js Foundation Build WorkGroup Meeting 2018-09-04 nodejs/build#1478

Closed

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

deps: fix CRLF in text file not present in upstream by joyeecheung · Pull Request #22340 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

deps: fix CRLF in text file not present in upstream#22340joyeecheung wants to merge 1 commit intonodejs:masternodejs/node:masterfrom joyeecheung:use-lf-in-v8joyeecheung/node:use-lf-in-v8Copy head branch name to clipboard

Conversation

joyeecheung commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Aug 15, 2018

Uh oh!

joyeecheung commented Aug 15, 2018

Uh oh!

joyeecheung commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Aug 15, 2018

Uh oh!

joyeecheung commented Aug 15, 2018

Uh oh!

refack commented Aug 15, 2018

Uh oh!

richardlau commented Aug 15, 2018

Uh oh!

joyeecheung commented Aug 15, 2018

Uh oh!

joyeecheung commented Aug 15, 2018

Uh oh!

joyeecheung commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Aug 15, 2018

Uh oh!

Trott commented Aug 15, 2018

Uh oh!

refack commented Aug 15, 2018

Uh oh!

joyeecheung commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Aug 16, 2018

Uh oh!

joyeecheung commented Aug 16, 2018

Uh oh!

richardlau commented Aug 16, 2018

Uh oh!

refack commented Aug 17, 2018

Uh oh!

jasnell commented Aug 17, 2018

Uh oh!

refack commented Aug 17, 2018

Uh oh!

refack commented Aug 17, 2018

Uh oh!

Trott commented Aug 17, 2018

Uh oh!

joyeecheung commented Aug 17, 2018

Uh oh!

refack commented Aug 17, 2018

Uh oh!

joyeecheung commented Aug 17, 2018

Uh oh!

joyeecheung commented Aug 17, 2018

Uh oh!

joyeecheung commented Aug 17, 2018

Uh oh!

joyeecheung commented Aug 17, 2018

Uh oh!

joyeecheung commented Aug 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

deps: fix CRLF in text file not present in upstream#22340
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:use-lf-in-v8

joyeecheung commented Aug 15, 2018 •
edited
Loading

joyeecheung commented Aug 15, 2018 •
edited
Loading

joyeecheung commented Aug 15, 2018 •
edited
Loading

joyeecheung commented Aug 16, 2018 •
edited
Loading

joyeecheung commented Aug 18, 2018 •
edited
Loading