Skip to content

Conversation

@mthalman
Copy link
Member

@mthalman mthalman commented Jan 9, 2023

This fixes the issue described in dotnet/source-build#3081 where source-build can encounter EOL TFMs during its build. This generates a warning which can then produce an error if warnings are treated as errors. We need to avoid having source-build be blocked by this situation.

So these changes disable the CheckEolTargetFramework MSBuild property globally for all projects. It also removes some the disabling of that property that was targeted for a couple specific projects.

Fixes dotnet/source-build#3081

@mthalman mthalman requested a review from a team as a code owner January 9, 2023 13:46
@crummel
Copy link

crummel commented Jan 10, 2023

If I remember correctly the reason we had this in F# environment variables was that the command line variables weren't actually being propagated all the way through the build. I think it might be safer to add an EnvironmentVariable entry in Directory.Build.props instead of the StandardSourceBuildArgs.

Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consider backporting this to 6.0. With 3.1 reaching EOL, I would bet we will see EOL failures once the SDK check is enabled in a couple months.

<StandardSourceBuildArgs>$(StandardSourceBuildArgs) /p:SourceBuildUseMonoRuntime=$(SourceBuildUseMonoRuntime)</StandardSourceBuildArgs>

<!-- https://github.com/dotnet/source-build/issues/3081 -->
<StandardSourceBuildArgs>$(StandardSourceBuildArgs) /p:CheckEolTargetFramework=false</StandardSourceBuildArgs>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work in most if not all cases. The reason is that build properties don't get automatically flown into the repo inner builds. An environment variable (example) would be the way to accomplish this given the way the system currently works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This documentation describes how the source-build works for a repo. The outer build properties do not flow into the inner build.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichaelSimons - Does this mean that the original content that I removed from xliff-tasks wasn't actually working as intended?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means that in main/.NET 8.0, they removed the EOL TFMs that were causing the build to break. If you were to backport these changes to .net 7.0 or 6.0, I would suspect you would get a build failure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...continuing that thought. I am sure you would get a build failure in FSharp. I can't answer why the xliff-tasks pattern works without looking at a binlog.

@mthalman mthalman changed the base branch from main to release/6.0.1xx January 10, 2023 19:53
@mthalman mthalman requested a review from rbhanda as a code owner January 10, 2023 19:53
@mthalman mthalman changed the base branch from release/6.0.1xx to main January 10, 2023 19:53
@mthalman mthalman changed the base branch from main to release/6.0.1xx January 10, 2023 20:00
@mthalman
Copy link
Member Author

Ok, I've updated to address the issue of flowing to the inner projects and also rebased to target release/6.0.1xx branch.

@MichaelSimons MichaelSimons merged commit d625ce4 into dotnet:release/6.0.1xx Jan 11, 2023
@mthalman mthalman deleted the sb-issue3081 branch January 11, 2023 22:27
MichaelSimons pushed a commit to MichaelSimons/installer that referenced this pull request Jan 31, 2023
MichaelSimons pushed a commit to MichaelSimons/installer that referenced this pull request Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider setting CheckEolTargetFramework=false globally

3 participants