Msbuild: Target framework attribute file should be written to IntermediateOutputPath

[jeffkl]

Currently GenerateTargetFrameworkMonikerAttribute defaults to write the target framework attribute file to %TEMP%. This means if multiple projects are building at once and the file doesn't exist, they can hit a race condition. The currently solution is to have the <WriteLinesToFile /> task marked to ContinueOnError which emits a warning.

I propose that we instead default to have the target framework attribute file be written to the IntermediateOutputPath which in most cases would be unique per project. The overhead on small projects would be very minimal and in large projects they would no longer have the race condition. I would change this line to say:

<TargetFrameworkMonikerAssemblyAttributesPath
    Condition="'$(TargetFrameworkMonikerAssemblyAttributesPath)' == ''">
    $([System.IO.Path]::Combine('$(IntermediateOutputPath)','$(TargetFrameworkMoniker).AssemblyAttributes$(DefaultLanguageSourceExtension)'))
</TargetFrameworkMonikerAssemblyAttributesPath>

FYI @jaredpar

[jaredpar]

That's exactly what we did in Roslyn and it seems to be working great

https://github.com/dotnet/roslyn/pull/15905

[jeffkl]

Related issue https://github.com/dotnet/roslyn/issues/10116

[yaakov-h]

@AndyGerlicher Is there anything technical or organizational holding this up for such a distant release, or would I be able to file a pull request for this change?

It looks to me like a simple change to src/Tasks/Microsoft.Common.CurrentVersion.targets.

[dasMulli]

Another complication: When written into /tmp in linux and not cleaned up, other users on a shared system may be unable to access the file and compilation will fail for these users. This impacts usage in schools and academic institutions.

https://stackoverflow.com/questions/45950644/prevent-net-core-2-0-from-leaving-files-in-tmp-on-rhel7

[noahplusplus]

On a shared institutional Linux system like ours, one problem is as @dasMulli described, with compilation failing. Another problem is lifecycle control: Our sysadmin does not want files in the global /tmp living beyond their useful life. He will not install .NET Core for us until both problems are solved.

Being "solved", from the sysadmin's perspective, entails a solution which applies to all current & future users at all times; which does not depend on user discipline; and which does not make impositions on any of the system's non-.NET users. So it cannot depend on the user remembering to override the built-in default in their project files, for instance.

It sounds like @jeffkl's proposal could solve both problems for us. Whether it does solve them depends on the default behavior of IntermediateOutputPath. If, by default, it always places its files inside the user's working directory, rather than in a shared space like $TMPDIR, then it solves our problems. I think this VS reference is saying that _its_ default is inside the project's obj directory. That would be good for us, too.

Another desirable property: the user not having to depend on developer discipline, either. A richer, but more complex solution might have a hierarchy of settings: a systemwide setting in /etc, with priority over a user setting, which itself has priority over any project setting. I wrote up a more complete proposal for this kind of scheme, but I am hoping @jeffkl's idea suits our needs as I think it would! Can anyone answer my question about IntermediateOutputPath?

[dasMulli]

I think there are two options here:

1. Use IntermediateOutputPath (=> obj\{Configuration}\{TFM}\) or BaseIntermediateOutputPath.
   
   
   
   • Note that the default filename is not project-unique so either keep the current "multiple projects may use it" code for shared intermediate output paths or rename the file ({projectName}.{TF},{TFV}.cs).
   
   
2. Use a different global directory (~/.dotnet/ comes to my mind which is already cluttered by a few .NET Core things).
   
   
   
   • maybe do it only on *nix?
   
   

Personally, I favour option 1 with a project-unique filename like My.Cool.Thing.NETCoreApp,v2.0.cs.
Downside: A bit more IO during first project load in tooling.

[yaakov-h]

1 sounds good to me.

2 would still throw an unnecessary warning when building projects in parallel, due to conflicts on the same shared file.

I don't think the unique file name is needed, since the obj path is already unique.

[dasMulli]

Getting rid of the warning is a good, forgot about that.

Some projects are configured to use a shared intermediate output path or even a shared base intermediate output path so if the file name is unique to the projects, there won't be conflicts. If it is not unique, then we're in the warning-instead-of-error and race condition handling business again 😢. Any change should not break these projects..

[yaakov-h]

I don't think any change proposed here would break them any more than they are broken today.

[omajid]

@dasMulli said:

I think there are two options here:

One other option - for Linux at least - is to use XDG_RUNTIME_DIR:

$XDG_RUNTIME_DIR defines the base directory relative to which user-specific non-essential runtime files and other file objects (such as sockets, named pipes, ...) should be stored. The directory MUST be owned by the user, and he MUST be the only one having read and write access to it. Its Unix access mode MUST be 0700.

If this data is not meant to be kept around, this is probably the best location on modern Linux systems.

[japj]

I saw this last week on one of our build servers where we do concurrent builds.
Not sure, but is the solution still blocked on something?

[tmat]

/cc @nguerrera
Why isn't this attribute (TargetFrameworkAttribute) generated to the AssemblyInfo.cs file like the other assembly level attributes?

[nguerrera]

@tmat It's different code that happens for all projects vs. generated assembly info being SDK project feature.

[tmat]

Can the SDK suppress it and write it to the AssemblyInfo.cs?

[maximpashuk]

I am agree to @tmat
Sdk-based projects have some autogenerated files:

• AssemblyInfo.cs with assembly attributes (AssemblyVersion, AssemblyTitle).
• RazorAssemblyInfo.cs with razor-related attributes (RazorLanguageVersion)
• UserSecretsAssemblyInfo.cs with UserSecretsId attribute

For TargetFramework attribute implementation can be the same:

• TargetFrameworkAssemblyInfo.cs, for example

keeping autogenerated .cs file with TargetFramework attribute in Temp folder making some troubles:

• race conditions as this issue described
• TeamCity internally cleaned Temp folder after each build, preventing incremental builds to work. Every TeamCity build call a csharp compiler, because *.cs not match output files (autogenerated file has been deleted)

@AndyGerlicher can you take a look at my comment?

[rainersigwald]

If someone is moved to open a PR for this I think we'd be willing to take it now for early v16 previews.

[jeffkl]

@rainersigwald I had a PR which didn't go very well: https://github.com/Microsoft/msbuild/pull/2571

[rainersigwald]

I think everything except the perf concerns is resolved by using IntermediateOutputDirectory (not Base...). And my guess is that the perf impact won't be very large, though I have no proof of that.

[jeffkl]

I think it should be a compiler argument but that wasn't a popular opinion. It seems so arbitrary to write a file to get an assembly attribute added.

[rainersigwald]

That'd work for me too, but I'd rather change this now, then add that feature, rather than waiting.

[Kazark]

I'm hitting what @maximpashuk noted:

TeamCity internally cleaned Temp folder after each build, preventing incremental builds to work. Every TeamCity build call a csharp compiler, because *.cs not match output files (autogenerated file has been deleted)

Is there a known workaround in the meantime?

[maximpashuk]

@Kazark

In root solution directory I created a file Directory.Build.props with following content

<Project>

  <!-- incremental builds in TeamCity -->
  <PropertyGroup>
    <TargetFrameworkMonikerAssemblyAttributesPath>obj\TargetFrameworkAttribute.cs</TargetFrameworkMonikerAssemblyAttributesPath>
  </PropertyGroup>

</Project>

Probably this is not a best solution, but it works, incremental builds now works in TeamCity as expected.

[Nirmal4G]

I vote for @tmat and @maximpashuk solution, intermediate dir all the things and into the Sdk, we go! 😁

[Kazark]

@maximpashuk Makes sense. Thanks. That should get me past it for now.

[KirillOsenkov]

@livarcocc would it be possible to prioritize this? This is an impactful issue that's very easy to fix and has been open for three years now. Seems like low-hanging fruit.

[bording]

With the fix for this being merged into master now, which release will it be a part of? I see that this issue is on the 16.5 milestone, so does that mean it will be in that release, or does the fix also need to be ported to another branch?

[tmat]

@bording I updated the milestone to 16.6 as I believe master is 16.6.

[Celess22]

https://stackoverflow.com/a/62146077/714557

[MagicAndre1981]

@bording I updated the milestone to 16.6 as I believe master is 16.6.

works now for me in 16.6.2. I see .NETStandard,Version=v2.0.AssemblyAttributes.cs in \project\obj\Debug\netstandard2.0