Skip to content

Clean up $(RepoRoot) consistently #32664

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
Merged
merged 1 commit into from
May 14, 2021
Merged

Clean up $(RepoRoot) consistently #32664

merged 1 commit into from
May 14, 2021

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented May 14, 2021

  • don't override correct values but fix incorrect ones

nits:

  • don't use $(MSBuildProjectDirectory) in project files
    • inconsistent w/ $(MSBuildThisFileDirectory) and harder to grok
  • don't add unnecessary slashes after $(MSBuildThisFileDirectory)
  • clean up Microsoft.AspNetCore.Testing.props
    • used only from eng/targets/CSharp.Common.props but fallback settings may help

@dougbu
Clean up $(RepoRoot) consistently
1e09d57 
- don't override correct values but fix incorrect ones

nits:
- don't use `$(MSBuildProjectDirectory)` in project files
  - inconsistent w/ `$(MSBuildThisFileDirectory)` and harder to grok
- don't add unnecessary slashes after `$(MSBuildThisFileDirectory)`
- clean up Microsoft.AspNetCore.Testing.props
  - used only from eng/targets/CSharp.Common.props but fallback settings may help
@dougbu dougbu requested review from SteveSandersonMS, captainsafia and a team May 14, 2021 03:56
@dougbu dougbu requested review from BrennanConroy, halter73 and a team as code owners May 14, 2021 03:56
@dougbu
Copy link
Contributor Author

dougbu commented May 14, 2021 •
edited
Loading

This slightly undoes #32552 and also cleans things up a bit more than I did there.

@captainsafia and @SteveSandersonMS this should address #32615. @SteveSandersonMS could you try your scenario while on this branch❔ If things work (and they should 😺), I'll add #32615 to the commit description when I merge.

@BrennanConroy I'm also testing that this fixes the SignalR pipeline problems. See https://dev.azure.com/dnceng/internal/_build/results?buildId=1138724 https://dev.azure.com/dnceng/internal/_build/results?buildId=1138761 for build w/ same branch.

dougbu
dougbu commented May 14, 2021
View reviewed changes
Directory.Build.props
@@ -3,7 +3,7 @@

<PropertyGroup>
<!-- $(RepoRoot) is normally set globally and Arcade overrides it to ensure a trailing slash. -->
<RepoRoot Condition=" '$(RepoRoot)' == '' ">$([MSBuild]::EnsureTrailingSlash('$(MSBuildThisFileDirectory)'))</RepoRoot>
<RepoRoot Condition=" '$(RepoRoot)' == '' OR !HasTrailingSlash('$(RepoRoot)') ">$(MSBuildThisFileDirectory)</RepoRoot>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I was mistaken thinking EnsureTrailingSlash(...) was needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rainersigwald one question: Why is this able to override %RepoRoot% in @SteveSandersonMS's #32615 scenario (under "Older report")❔ Are environment variables able to be overridden in msbuild files even though they're otherwise treated as global properties❔

Copy link
Member

Choose a reason for hiding this comment

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

Environment variables are treated as normal properties, not global properties, so they're mutable, they just have an initial value at the very beginning of evaluation.

src/SignalR/clients/ts/FunctionalTests/SignalR.Npm.FunctionalTests.npmproj
@@ -31,7 +31,7 @@
<!-- Skip the "inner" test run when we're running DailyTests -->
<Yarn Command="run test:inner --no-color --configuration $(Configuration)"
Condition="'$(DailyTests)' != 'true'"
WorkingDirectory="$(RepoRoot)src/SignalR/clients/ts/FunctionalTests" />
WorkingDirectory="$(MSBuildThisFileDirectory)" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a teensy nit. No reason to describe the path in so much detail when $(MSBuildThisFileDirectory) is sitting right there…

@dougbu dougbu mentioned this pull request May 14, 2021
Closed
@dougbu
Copy link
Contributor Author

dougbu commented May 14, 2021 •
edited
Loading

So far,

  1. CI build looking good other than RazorSourceGenerator issues seen elsewhere
  2. I didn't update AzDO correctly, started https://dev.azure.com/dnceng/internal/_build/results?buildId=1138761 w/ the correct branch content

@dougbu dougbu mentioned this pull request May 14, 2021
Closed
@dougbu
Copy link
Contributor Author

dougbu commented May 14, 2021

No need for further checks @SteveSandersonMS. I reproduced #32615 locally in a cmd window and then demonstrated this branch fixes the problem. See lots of lovely bits like the following a binary log after using dotnet build /bl:

Property reassignment: $(RepoRoot)="C:\dd\dnx\w\aspnetcore\" (previous value: "C:\dd\dnx\w\aspnetcore\src\Components\..\..") at C:\dd\dnx\w\aspnetcore\Directory.Build.props (6,5)

@dougbu
Copy link
Contributor Author

dougbu commented May 14, 2021

Just need an approval and the build to complete. Validated fix for #32615 and https://dev.azure.com/dnceng/internal/_build/results?buildId=1138761 went fine.

@dougbu dougbu merged commit c53700b into main May 14, 2021
@dougbu dougbu deleted the dougbu/signalarggh branch May 14, 2021 06:21
@ghost ghost added this to the 6.0-preview5 milestone May 14, 2021
@SteveSandersonMS
Copy link
Member

Thanks @dougbu and @captainsafia for resolving this!

BrennanConroy
BrennanConroy reviewed May 14, 2021
View reviewed changes
src/Testing/src/build/Microsoft.AspNetCore.Testing.props
@@ -1,9 +1,13 @@
<Project>
<PropertyGroup>
<RepoRoot
Condition=" '$(RepoRoot)' == '' OR !HasTrailingSlash('$(RepoRoot)') "><RepoRoot>$([MSBuild]::NormalizeDirectory('$(MSBuildThisFileDirectory)', '..', '..', '..', '..'))</RepoRoot></RepoRoot>
Copy link
Member

Choose a reason for hiding this comment

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

Is <RepoRoot><RepoRoot>...</RepoRoot></RepoRoot> a thing or just a typo?

Copy link
Member

Choose a reason for hiding this comment

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

@dougbu I'm also wondering about this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a typo and a dumb one -- cut and paste error ☹️ Fortunately the outer Condition is rarely, if ever true. PR incoming…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in #32680

dougbu added a commit that referenced this pull request May 14, 2021
@dougbu
Remove inner <RepoRoot/> element
7f0f6fa 
- typo introduced in #32664
- see discussion at #32664 (comment)
@dougbu dougbu mentioned this pull request May 14, 2021
Merged
ghost pushed a commit that referenced this pull request May 14, 2021
@dougbu
Remove inner <RepoRoot/> element (#32680)
db4ef86 
- typo introduced in #32664
- see discussion at #32664 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rainersigwald rainersigwald rainersigwald left review comments

@BrennanConroy BrennanConroy BrennanConroy left review comments

@wtgodbe wtgodbe wtgodbe left review comments

@HaoK HaoK HaoK approved these changes

@SteveSandersonMS SteveSandersonMS Awaiting requested review from SteveSandersonMS

@captainsafia captainsafia Awaiting requested review from captainsafia

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.0-preview5
Development

Successfully merging this pull request may close these issues.
6 participants
@dougbu @SteveSandersonMS @rainersigwald @HaoK @BrennanConroy @wtgodbe