Msbuild: Long term effort for getting off Environment variables (for reproducible builds)

[KirillOsenkov]

The fact that MSBuild initializes properties with environment variables is very bad for reproducible builds. CI servers often have different sets of environment variables set, and investigating on the CI server is usually hard.

For compatibility reasons, I doubt we'll ever get rid of reading environment, but I hope there can be "Strict Mode" where environment is not read at all. Builds could opt in to strict mode and use other means of initializing global state.

Example:
https://github.com/NuGet/NuGet.Client/blob/2aad8d25e66e9c3737b220631f3661bb6235d14b/src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets#L555-L565

Imagine if the CI Server defines the VERSION environment variable and sets it to something like a Git SHA.

You get a build failure like this:

/Library/Frameworks/Mono.framework/Versions/5.8.0/lib/mono/xbuild/NuGet.targets(102,5): error : 'fbc5117dcb21f0d180c4b199d828d4f4b9d6f4a8' is not a valid version string. [/Users/builder/data/lanes/5668/fbc5117d/source/md-addins/MacSetup/build/BinaryChecker.proj]
/Library/Frameworks/Mono.framework/Versions/5.8.0/lib/mono/xbuild/NuGet.targets(102,5): error : Parameter name: value [/Users/builder/data/lanes/5668/fbc5117d/source/md-addins/MacSetup/build/BinaryChecker.proj]

Tracking it down from that failure to that line to NuGet.targets is highly non-trivial.

[KirillOsenkov]

Example issue: https://github.com/NuGet/Home/issues/6154

[nguerrera]

1144

[david-driscoll]

Years ago when I got a new PC at work (an hp), as part of the default install of windows (from hp or internally not sure) there were a few environment variables that were set, one was PLATFORM which completely hosed visual studio until I figured it out.

[dasMulli]

Related: https://github.com/dotnet/sdk/issues/481

Also looks like some folks are using VERSION env vars on docker images that throw off builds. Was diagnosing the impact and workarounds with @niemyjski - e.g. TreatAsLocalProperty.

[kzu]

Just like /noautoresponse, I'd name this something like /noenvironment or /novars or (uglier?) /noenvvars

[nguerrera]

Opting out of the specific behavior was what I was suggesting in #1144

[sandyarmstrong]

Another example issue: https://github.com/NuGet/Home/issues/5614

[jaredpar]

Want to add my most recent pain point here. I've been working on moving the Roslyn infrastructure from Jenkins to VSTS. When defining my initial YAML file I made the fateful decision to name on of the YAML matrix variables "configuration".

As a result this ended up adding an environment variable called "configuration" which was then present for our build and subsequent running of unit tests. The unit tests included a set of MSBuild integration tests which happily inherited this environment variable, set some defaults and ended up breaking our tests in a really subtle way. Took three days of digging to get to the bottom of this one.

[cdmihai]

We talked this over, and we're interested in introducing:

• explicit environment reads with a shorthand [Env]::Get("Path") (same as the current [System.Environment]::GetEnvironmentProperty())
• a property lookup fallback mechanism. I think the order would be reserved properties -> global properties -> defined properties -> environment properties. Right now these just get dumped in a global scope in the right order.
• option to turn off implicit environment reads
• option to error on implicit environment reads
• log implicit environment reads

[abock]

• option to turn off implicit environment reads
• option to error on implicit environment reads

I'd love to see a mechanism for this to be controlled _without_ having to change how msbuild is invoked, preferably via some new reserved MSBuild properties (e.g. I would set these properties in Directory.Build.props).

I realize this might have some tricky implications (e.g. setting the property conditionally based on an implicit environment read), but allowing this behavior only via command line options will only confuse the matter.

Perhaps any condition expressions applied to reserved properties would _never_ allow implicit environment reads?

export SOME_ENV_VAR=foo

<PropertyGroup>
  <MSBuildDisableImplicitEnvironmentProperties>true</MSBuildDisableImplicitEnvironmentProperties>
  <MSBuildDisableImplicitEnvironmentProperties Condtion="'$(SOME_ENV_VAR)' != ''">false</MSBuildDisableImplicitEnvironmentProperties>
</PropertyGroup>

Since MSBuildDisableImplicitEnvironmentProperties is "reserved", $(SOME_ENV_VAR) would never be resolved via environment, and thus the original value of the property would remain true.

[rainersigwald]

A problem with setting MSBuildDisableImplicitEnvironmentProperties is dealing with what happens _before_ that property is encountered. What if it's the last property defined in a project, after several environment reads?

[jaredpar]

A problem with setting MSBuildDisableImplicitEnvironmentProperties is dealing with what happens before that property is encountered.

Indeed. I don't think it can realistically be done as a property. Unless the semantics were instead "no more enviornment reads after this is set" which doesn't seem as useful.

What if it's the last property defined in a project, after several environment reads?

So I'm going to say this and immediately duck: could it be in global.json?

[abock]

Right, that's why I said it's a bit tricky 😄. This would almost need to be pre-processed.

The concern I have though is that I want my projects to define how MSBuild behaves - not the command line. The user experience for devs should just be git clone and then msbuild or dotnet build and not having to worry about which project needs which set of command line options passed to it.

In the Unix world, most projects can be built simply with ./configure && make. I strive to provide a similar experience for my .NET projects as well.

Another alternative could simply be an "implied" response file.

👉tree MyProject
MyProject
├── Directory.Build.props
├── Directory.Build.rsp
├── Project.sln
└── README.md

👉cat MyProject/Directory.Build.rsp
/nologo
/disableImplicitEnvironmentProperties
/strict
/etc

[abock]

💯 WOW, TIL

  @<file>            Insert command-line settings from a text file. To specify
                     multiple response files, specify each response file
                     separately.

                     Any response files named "msbuild.rsp" are automatically
                     consumed from the following locations:
                     (1) the directory of msbuild.exe
                     (2) the directory of the first project or solution buil

[abock]

So I'm going to say this and immediately duck: could it be in global.json?

This would honestly be great, since we already have this file pretty much everywhere now. I love the new (to me, clearly) auto-response file, so perhaps we could do the same for global.json, just so there isn't yet another file at the root of the repo, etc.

[rainersigwald]

@abock There is support for Directory.Build.rsp: https://docs.microsoft.com/en-us/visualstudio/msbuild/customize-your-build#add-arguments-to-command-line-msbuild-invocations-for-your-project (implemented by #2627).

global.json

_hisses, skitters away_

[xoofx]

Just ran again into this issue, not the first time, but for example, someone in our company had the "VERSION" env variable setup... and it was messing the entire build, changing the implicit version of all of our assemblies, and breaking many of our tests.

But I have been through also many times in the past with some weird errors caused by similar env variable conflicting issues...

So I'm all for /disableImplicitEnvironmentProperties as suggested @abock

I'm keen to open a PR if it is possible to bring that option.

[rainersigwald]

@xoofx Sure, I'll assign this to you. Can you sketch a quick plan for the implementation you're thinking of before diving in all the way? Some possibly-not-obvious things to think about: API exposure (is it just a new BuildParameter?), node reuse (must not be kept in persistent state), reporting interaction with #5038.

Also note: if this is command-line driven, it will NOT apply to builds in Visual Studio. So it's still an improvement but might result in some confusing command-line vs. UI-driven build differences.

[xoofx]

@xoofx Sure, I'll assign this to you. Can you sketch a quick plan for the implementation you're thinking of before diving in all the way? Some possibly-not-obvious things to think about: API exposure (is it just a new BuildParameter?), node reuse (must not be kept in persistent state), reporting interaction with #5038.

I don't know yet, I haven't looked at the detail (I come to msbuild code base once every 2 years, so I'm not super fluent with it 😅 ). But assuming that this would add a new parameter /disableImplicitEnvironmentProperties that's what you mean by BuildParameter?
Ok for node reuse hope it won't be a trouble to disable that in that case.
And yep for #5038, I saw that.

Also note: if this is command-line driven, it will NOT apply to builds in Visual Studio. So it's still an improvement but might result in some confusing command-line vs. UI-driven build differences.

Indeed, but I would assume that we could then later bring support for that inside Visual Studio (e.g as an option in build properties page), separately from this PR, right?

[rainersigwald]

MSBuild's command-line parameters are defined in https://github.com/microsoft/msbuild/blob/master/src/MSBuild/CommandLineSwitches.cs. They're mostly used to influence the call to the MSBuild API that MSBuild.exe makes, like so:

https://github.com/microsoft/msbuild/blob/96403c598a166e5a06933f78ace75528a1d1f3b0/src/MSBuild/XMake.cs#L1147-L1158

So implementing this would involve making it possible to express in the API, then connecting a new command line argument to that.

Yes, after it's implemented in the API we can separately request a VS UI option to set that flag in its own calls to our API.