Skip to content

Fix MakeRelative regression intrdouced in msbuild shipped with VS16.10#6504

Closed
pmisik wants to merge 1 commit intodotnet:vs16.10from
pmisik:makerelative_fix
Closed

Fix MakeRelative regression intrdouced in msbuild shipped with VS16.10#6504
pmisik wants to merge 1 commit intodotnet:vs16.10from
pmisik:makerelative_fix

Conversation

@pmisik
Copy link
Copy Markdown

@pmisik pmisik commented Jun 1, 2021

Fixes #6493

Context

Changes Made

Testing

Added missing directory related unittests

Notes

@rainersigwald
Copy link
Copy Markdown
Member

Can you rebase this off the vs16.10 branch so we can (potentially, not promising yet) take it for a patch release of 16.10?

@dnfadmin
Copy link
Copy Markdown

dnfadmin commented Jun 1, 2021

CLA assistant check
All CLA requirements met.

@dnfadmin
Copy link
Copy Markdown

dnfadmin commented Jun 1, 2021

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ pmisik sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@pmisik
Copy link
Copy Markdown
Author

pmisik commented Jun 1, 2021

Can you rebase this off the vs16.10 branch so we can (potentially, not promising yet) take it for a patch release of 16.10?

I'll do it if after it pass CI.
First step was to break unittests (as directory related unittests were missing) to prove it has broken previous MSBuild.
Second commit is fix based on @dsparkplug analysis.
I hope it will fix it.
Unfortunately I cannot run it locally on my machine as I see
Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(141,5): error NETSDK1045: The current .NET SDK does not support targeting .NET 6.0. Either target .NET 5.0 or lower, or use a version of the .NET SDK that supports .NET 6.0.

@rainersigwald
Copy link
Copy Markdown
Member

Unfortunately I cannot run it locally on my machine as I see
Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(141,5): error NETSDK1045: The current .NET SDK does not support targeting .NET 6.0. Either target .NET 5.0 or lower, or use a version of the .NET SDK that supports .NET 6.0.

To fix that, either rebase (to get back to a branch that targets .NET 5.0), or install the latest .NET 6.0 preview SDK. I recommend the former :)

@dsparkplug
Copy link
Copy Markdown

While this patch will fix the issue with directories, a directory separator will be added to the end of the output for all input, causing the existing unit tests for files to fail. A solution would need to check whether path is a file.

@dsparkplug
Copy link
Copy Markdown

Added a new pull request to replace this one #6508

@pmisik pmisik force-pushed the makerelative_fix branch from 9b9c5cb to 2615c87 Compare June 2, 2021 20:10
@pmisik pmisik changed the base branch from main to vs16.10 June 2, 2021 20:16
@pmisik pmisik changed the title [WIP] Fix MakeRelative regression intrdouced in msbuild shipped with VS16.10 Fix MakeRelative regression intrdouced in msbuild shipped with VS16.10 Jun 2, 2021
@pmisik
Copy link
Copy Markdown
Author

pmisik commented Jun 2, 2021

Can you rebase this off the vs16.10 branch so we can (potentially, not promising yet) take it for a patch release of 16.10?

I've rebased and squashed (to me intermediate commit are useless - single commits on production branches are better to find cause of issue) work to be potentially applied on 16.10 branch.
I vote for fix in VS16.10.
For company I work at this is blocker bug (we have many supported target branches with different vc toolsets).
Unfortunately, there is no support in VS2019 to have multiple msbuild versions side by side for various VC toolsets (the same issue applies on WDK - as it overwrites original one build sub-directory :-( ).
So, if any developer at company install VS 16.10 it will break all existing branches development for him.

Should it be as new feature request for VS2022 to have support side by side msbuild suppport?

After squash I amended commit message to be explicitly visible all credits to this fix belongs to @dsparkplug
Author and committer should belongs to @dsparkplug.
Mine only intentions is to be fixed this issue ASAP in production.

@rainersigwald
Copy link
Copy Markdown
Member

I vote for fix in VS16.10.
For company I work at this is blocker bug (we have many supported target branches with different vc toolsets).
Unfortunately, there is no support in VS2019 to have multiple msbuild versions side by side for various VC toolsets (the same issue applies on WDK - as it overwrites original one build sub-directory :-( ).
So, if any developer at company install VS 16.10 it will break all existing branches development for him.

Thanks for this--this kind of detailed problem description helps us get approval for servicing bugs. We'll let you know the outcome of that process (I'm optimistic).

@Forgind
Copy link
Copy Markdown
Contributor

Forgind commented Jun 14, 2021

#6513 had the key change from this PR, so closing as superseded by that. Thanks for the contribution @pmisik!

@Forgind Forgind closed this Jun 14, 2021
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.

$([MSBuild]::MakeRelative()) returns different values on 16.10 than older version

5 participants