-
Notifications
You must be signed in to change notification settings - Fork 585
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
Disable NodeReuse by default (FAKE 4) #1600
Disable NodeReuse by default (FAKE 4) #1600
Conversation
My opinion: n̗ͬò̱ͣ̏̓ͦd̤̄̑e̘̻͔̻̣͇̣ ͖̣̹͍̆̂̃̔͌ͭr͓̽̾e̱̳̹̯͕ͮͨ͂u͇̜̺̦̳͈ͪs͚̖̣͇̒͐͆̚ĕ͓̥̯̔̏ͧ is evil, and should always be disabled. I've had so many issues with this inside Visual Studio that I have set the global environment variable to disable it. If I had the permissions, I would add a global domain group policy to force this variable (and disable node reuse) on all machines. |
@0x53A: I did not want to put it that drastic, but yes: we had a lot of problems with node reuse (i.e. when changing framework versions and moving between branches of different versions) and would like to remove it. While looking for ways to do this in FAKE, I came across this default for a few arbitrary CI servers and wanted to make it consistent across all CI servers. I do not know about the performance impact, but I would not mind disabling it completely as well (by default). |
I think disable by default is probably fine. I had problems before with it on my local machine as well. Please send a PR for https://github.com/fsharp/FAKE/blob/master/src/app/Fake.DotNet.MsBuild/MsBuild.fs#L242 as well (info the master branch) |
I have now just set it to false by default and also created a PR for the master branch. As a side note: I would like to see this merged as quickly as possible, as we are experiencing problems on our build servers with node reuse |
I thought you could just set the parameter to false for now? |
that's not that easy, as the params are hidden in a large cascase of build calls (unleass I am mistaken and have missed something) |
Released in https://www.nuget.org/packages/FAKE/4.63.0 |
thank you! |
The default for NodeReuse was a bit arbitrary, as it only considered TeamCity, TFS and Jenkins. As we were experiencing problems with NodeReuse on GitLab as well, I suggest to disable this for alle build servers (=> only enable for local builds).
Most likely, environments like AppVeyor, where a new instance is created per build job don't have a problem with this, but as they have so many things being run anyway, I would not expect a measurable performance penalty.
Please gibe any comments if this is acceptable or what else would be to consider. If this is accepted, shall I prepare a PR for the v5/master branch as well?