Skip to content
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

Target framework attribute file should be written to IntermediateOutputPath #1479

Closed
jeffkl opened this issue Dec 14, 2016 · 29 comments · Fixed by #5101
Closed

Target framework attribute file should be written to IntermediateOutputPath #1479

jeffkl opened this issue Dec 14, 2016 · 29 comments · Fixed by #5101
Labels
bug help wanted Issues that the core team doesn't plan to work on, but would accept a PR for. Comment to claim. triaged

Comments

@jeffkl
Copy link
Contributor

jeffkl commented Dec 14, 2016

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
Copy link
Member

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

dotnet/roslyn#15905

@jeffkl
Copy link
Contributor Author

jeffkl commented Dec 14, 2016

Related issue dotnet/roslyn#10116

@yaakov-h
Copy link
Member

yaakov-h commented May 4, 2017

@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
Copy link
Contributor

dasMulli commented Aug 30, 2017

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
Copy link

noahplusplus commented Sep 4, 2017

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
Copy link
Contributor

dasMulli commented Sep 4, 2017

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
Copy link
Member

yaakov-h commented Sep 4, 2017

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
Copy link
Contributor

dasMulli commented Sep 4, 2017

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
Copy link
Member

yaakov-h commented Sep 4, 2017

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

@omajid
Copy link
Member

omajid commented Sep 5, 2017

@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
Copy link

japj commented Nov 4, 2017

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?

@AndyGerlicher AndyGerlicher modified the milestones: MSBuild 15.5, After 15 Nov 6, 2017
@tmat
Copy link
Member

tmat commented Nov 28, 2017

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

tmat added a commit to tmat/roslyn-tools that referenced this issue Nov 28, 2017
tmat added a commit to dotnet/roslyn-tools that referenced this issue Nov 28, 2017
@nguerrera
Copy link
Contributor

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

@tmat
Copy link
Member

tmat commented Nov 28, 2017

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

@maximpashuk
Copy link

maximpashuk commented Oct 25, 2018

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?

@Kazark
Copy link

Kazark commented Jul 17, 2019

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
Copy link

maximpashuk commented Jul 18, 2019

@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
Copy link
Contributor

Nirmal4G commented Jul 18, 2019

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

@Kazark
Copy link

Kazark commented Jul 18, 2019

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

@KirillOsenkov
Copy link
Member

@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
Copy link
Contributor

bording commented Feb 12, 2020

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 tmat modified the milestones: MSBuild 16.5, MSBuild 16.6 Feb 12, 2020
@tmat
Copy link
Member

tmat commented Feb 12, 2020

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

@BearSleepy
Copy link

@MagicAndre1981
Copy link

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help wanted Issues that the core team doesn't plan to work on, but would accept a PR for. Comment to claim. triaged
Projects
None yet