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.

Sign up for GitHub

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

Jump to bottom

Revert broken code cleanup #6245

Merged
merged 2 commits into from Mar 15, 2021
Merged

Revert broken code cleanup #6245

merged 2 commits into from Mar 15, 2021

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Mar 11, 2021

Fixes internally reported bug

Context

EnsureTrailingSlash and HasTrailingSlash use slightly different escaping mechanisms in some cases. Reverting to prevent failures.

cdmihai
cdmihai approved these changes Mar 13, 2021
View reviewed changes
Copy link
Contributor

@cdmihai cdmihai left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe followup with an issue to define their behaviour and make them work as expected.

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Mar 15, 2021
@benvillalobos benvillalobos merged commit 0e4b0c2 into dotnet:master Mar 15, 2021
@Forgind Forgind deleted the revert-ensure-trailing-slash branch March 15, 2021 16:36
@Nirmal4G
Copy link
Contributor

Changes are from #5238
Follow up with #6251

Nirmal4G
Nirmal4G reviewed Mar 21, 2021
View reviewed changes
Copy link
Contributor

@Nirmal4G Nirmal4G left a comment

Choose a reason for hiding this comment

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

Were the following line changes also got affected by the *TrailingSlash difference? To me, it seems unrelated to the bug!

Sorry for reviewing this late.

src/Tasks/Microsoft.Common.CurrentVersion.targets
Comment on lines -154 to -155
<OutputPath Condition="'$(OutputPath)' == '' and '$(PlatformName)' == 'AnyCPU'">$([System.IO.Path]::Combine('$(BaseOutputPath)', '$(Configuration)'))</OutputPath>
<OutputPath Condition="'$(OutputPath)' == '' and '$(PlatformName)' != 'AnyCPU'">$([System.IO.Path]::Combine('$(BaseOutputPath)', '$(PlatformName)', '$(Configuration)'))</OutputPath>
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines seem unrelated to *TrailingSlash difference bug!!

src/Tasks/Microsoft.Common.CurrentVersion.targets
Comment on lines -159 to -160
<IntermediateOutputPath Condition="'$(IntermediateOutputPath)' == '' and '$(PlatformName)' == 'AnyCPU'">$([System.IO.Path]::Combine('$(BaseIntermediateOutputPath)', '$(Configuration)'))</IntermediateOutputPath>
<IntermediateOutputPath Condition="'$(IntermediateOutputPath)' == '' and '$(PlatformName)' != 'AnyCPU'">$([System.IO.Path]::Combine('$(BaseIntermediateOutputPath)', '$(PlatformName)', '$(Configuration)'))</IntermediateOutputPath>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same...

src/Tasks/Microsoft.Common.CurrentVersion.targets
Comment on lines -406 to +409
<_WinMDDebugSymbolsOutputPath>$(OutDir)$([System.IO.Path]::GetFileName('$(WinMDExpOutputPdb)'))</_WinMDDebugSymbolsOutputPath>
<_WinMDDebugSymbolsOutputPath>$([System.IO.Path]::Combine('$(OutDir)', $([System.IO.Path]::GetFileName('$(WinMDExpOutputPdb)'))))</_WinMDDebugSymbolsOutputPath>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here...

src/Tasks/Microsoft.Common.CurrentVersion.targets
Comment on lines -411 to +414
<_WinMDDocFileOutputPath>$(OutDir)$([System.IO.Path]::GetFileName('$(WinMDOutputDocumentationFile)'))</_WinMDDocFileOutputPath>
<_WinMDDocFileOutputPath>$([System.IO.Path]::Combine('$(OutDir)', $([System.IO.Path]::GetFileName('$(WinMDOutputDocumentationFile)'))))</_WinMDDocFileOutputPath>
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, here...

src/Tasks/Microsoft.Common.CurrentVersion.targets
Comment on lines -3412 to +3416
<TargetFrameworkMonikerAssemblyAttributesPath Condition="'$(TargetFrameworkMonikerAssemblyAttributesPath)' == ''">$(IntermediateOutputPath)$(TargetFrameworkMoniker).AssemblyAttributes$(DefaultLanguageSourceExtension)</TargetFrameworkMonikerAssemblyAttributesPath>
<TargetFrameworkMonikerAssemblyAttributesPath Condition="'$(TargetFrameworkMonikerAssemblyAttributesPath)' == ''">$([System.IO.Path]::Combine('$(IntermediateOutputPath)','$(TargetFrameworkMoniker).AssemblyAttributes$(DefaultLanguageSourceExtension)'))</TargetFrameworkMonikerAssemblyAttributesPath>
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too!

@Forgind
Copy link
Member Author

Forgind commented Mar 22, 2021

@Nirmal4G, You're right that this hit more lines than was necessary. EnsureTrailingSlash(x) and !HasTrailingSlash(x) ? x\ look obviously identical to me in all relevant ways, but for some reason they weren't, and I couldn't figure out why even the differences I could find could explain the symptom—OutputPath wasn't just slightly wrong but reset to the default value. If I could have told you exactly what the bug was, I would have just resolved that, but I decided it was safer to take out all the possibly-buggy code clarity changes from that commit rather than risk another high-priority bug. That said, if you can figure out what the root of the issue with EnsureTrailingSlash is, I'd be happy to bring these all back after resolving it. I'm sorry if that answer isn't satisfactory.

For what it's worth, I think the code looked better before this change, but I'd prefer ugly code over broken code.

@Nirmal4G
Copy link
Contributor

Is it possible to repro? I could take a look at. I'm thinking that this could be related to #5238 (comment)

@Forgind
Copy link
Member Author

Forgind commented Mar 22, 2021

@John-Hart 👆

I think the answer was yes, but it's hard, so maybe no. But I'll let the expert speak for himself.

@John-Hart
Copy link

It's not real easy to repro this. We were seeing this from our VS extension where we have code similar to:

Microsoft.Build.Evaluation.Project project = new Microsoft.Build.Evaluation.Project(projectPath, m_buildProperties, null);
// Redirect output to a separate temp folder
project.SetProperty("OutputPath", "c:\temp\out");
string outputpath = project.GetPropertyValue("OutputPath");
project.Build();
string updatedOutputpath = project.GetPropertyValue("OutputPath");
Debug.Assert(String.Equals(outputpath, updatedOutputpath);

Nirmal4G added a commit to Nirmal4G/MSBuild that referenced this pull request Jan 23, 2022
@Nirmal4G
Do use MSBuild's Path-specific property functions (Reverts dotnet#6245)
275f87f 
This reverts commit 0e4b0c2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nirmal4G Nirmal4G Nirmal4G left review comments

@KirillOsenkov KirillOsenkov KirillOsenkov approved these changes

@cdmihai cdmihai cdmihai approved these changes

Assignees
No one assigned
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Forgind @Nirmal4G @John-Hart @KirillOsenkov @cdmihai @benvillalobos