-
Notifications
You must be signed in to change notification settings - Fork 165
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
Setting autocrlf
and safecrlf
on workers
#1453
Comments
Maybe we should just put the configuration in https://github.com/nodejs/build/blob/master/jenkins/scripts/node-test-commit-pre.sh instead? |
+1 for configuration changes in scripts rather than manually done. |
Also, as far as I can tell the containers have their configs set in the Jenkins instead of using the script |
@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 |
@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 |
Does that work with https://github.com/nodejs/node/blob/8a41470c85536119a6dbeaf4b40dc9ebc4c453c4/deps/v8/.gitattributes#L1-L2 ? It's a file in |
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 Please let me take a better look at this, I'd much rather find a solution that doesn't involve changing machine configuration. |
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 |
That sounds good to me, if @targos is ok with it. |
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 |
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. |
Given that we have actually broken this in the past this would be better off as a separate job (that actually unpacks the tarball and attempts to compile/test it). |
`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>
`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>
I run this groovy script: import hudson.util.RemotingDiagnostics
import jenkins.model.Jenkins
String agent_name = 'test-digitalocean-ubuntu1604_sharedlibs_container-x64-3'
//groovy script you want executed on an agent
String groovy_script = '''
println "git config --global core.autocrlf false".execute().text
println "git config --global core.safecrlf false".execute().text
println "git config --global -l".execute().text
'''.trim()
Jenkins.instance.slaves.findAll { agent ->
agent.name ==~ /(?i).*container.*/
}.forEach { agent ->
println "################## ${agent.name} #################"
println RemotingDiagnostics.executeGroovy(groovy_script, agent.channel)
println '####################'
} to get:
|
Also for good measure run with
|
`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>
Help, this can't be killed:
|
So apparently with git 1.8 we need to delete the workspace for the settings to work. |
I thought the errors would not appear if you are checking out from a commit without the CRLF file and not rebasing from somewhere that contains it? (but yeah, it'll appear the next time we have a file like that in deps/v8) |
I was testing explicitly with "bad" commits:
https://ci.nodejs.org/job/node-test-commit-linuxone/4190/ When everything is setup right, it passes. |
`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>
This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made. |
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
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
The text was updated successfully, but these errors were encountered: