Setting `autocrlf` and `safecrlf` on workers

[joyeecheung]

For context, see nodejs/reliability#12 and the minutes of the last build meeting #1448 It was agreed to fix those workers by configuring git to not fix the line breaks.

I'll fix the nodes manually by running

git config --global core.autocrlf false
git config --global core.safecrlf false

on nodes with trouble checking out files with CRLF.

Feel free to add nodes in the following list if you find a new worker with this issue or have fixed a worker

• test-packetnet-ubuntu1604-arm64-2
• test-softlayer-ubuntu1604_sharedlibs_container-x64-2
• test-softlayer-ubuntu1604_sharedlibs_container-x64-5
• test-softlayer-ubuntu1604_sharedlibs_container-x64-1
• test-linuxonecc-rhel72-s390x-2
• test-packetnet-centos7-arm64-2
• test-joyent-smartos16-x64-1
• test-digitalocean-ubuntu1604_sharedlibs_container-x64-1
• test-packetnet-centos7-arm64-1
• test-linuxonecc-rhel72-s390x-3
• test-digitalocean-ubuntu1604_sharedlibs_container-x64-3
• test-digitalocean-ubuntu1604_sharedlibs_container-x64-10

[joyeecheung]

Maybe we should just put the configuration in https://github.com/nodejs/build/blob/master/jenkins/scripts/node-test-commit-pre.sh instead?

[richardlau]

+1 for configuration changes in scripts rather than manually done.

[joyeecheung]

Also, as far as I can tell the containers have their configs set in the Jenkins instead of using the script

[joaocgreis]

autocrlf should not be changed in the Windows machines, we want to ensure new contributors can compile with a default installation of Git (we already ask for the Unix tools, but the less of those little details the better and some people might not want to change it). I understand this does not include Windows hosts for now, but there is support to run the V8 tests on Windows and the ChakraCore nightlies seem to be failing because of this.

@joyeecheung can we instead use https://github.com/nodejs/node/blob/master/.gitattributes to specify that that file is always CRLF? This is already done for vcbuild.bat.

[joyeecheung]

@joaocgreis The failures in CI are caused by incorrect checkouts (not a fresh clone of nodejs/node), I don't see changing settings on our CI makes a difference since that's not how new contributors set up their environment anyway? For instance, developers don't see the modified: deps/v8/third_party/jinja2/LICENSE error, but we do.

[joyeecheung]

can we instead use https://github.com/nodejs/node/blob/master/.gitattributes to specify that that file is always CRLF? This is already done for vcbuild.bat.

Does that work with https://github.com/nodejs/node/blob/8a41470c85536119a6dbeaf4b40dc9ebc4c453c4/deps/v8/.gitattributes#L1-L2 ? It's a file in deps/v8 but not actually in V8's upstream source, and V8 actually uses LF for all its own text files in its own .gitattributes

[joaocgreis]

I don't see changing settings on our CI makes a difference since that's not how new contributors set up their environment anyway?

It should be, Windows CI machines are set up in the same way that we recommend contributors to set their own (with a few additions for Jenkins). Using a different autocrlf setting in CI can cause tests to fail because of different line endings.

Please let me take a better look at this, I'd much rather find a solution that doesn't involve changing machine configuration.

[joyeecheung]

As it's a file maintained by us and not in V8's source, maybe we can just patch it to use LF? That way it won't conflict with deps/v8/.gitattributes

[joaocgreis]

As it's a file maintained by us and not in V8's source, maybe we can just patch it to use LF?

That sounds good to me, if @targos is ok with it.

[joaocgreis]

I confirmed this is only a problem depending on the Git version. Happens with older versions (tested 1.9.1) but not with newer (2.18.0). nodejs/node#22340 (thanks for opening @joyeecheung!) should fix the problem, but in any case I'd be ok with changing the crlf settings only in machines where Git can't be updated.

[refack]

we want to ensure new contributors can compile with a default installation of Git

That was my motivation as well. Setting git to not touch the files in any way would break Windows build in case file are improperly checked in (e.g. .bat file with just LF). So we will be actually testing if the code can compile even from tarball.