Skip to content

/ aspnetcore

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

Jump to bottom

Clean up $(RepoRoot) consistently #32664

Merged
merged 1 commit into from May 14, 2021
Merged

Clean up $(RepoRoot) consistently #32664

merged 1 commit into from May 14, 2021
+26 −21

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
Verified
This commit was signed with the committer’s verified signature.
dougbu Doug Bunting
GPG key ID: EE41520987982C03 Learn about vigilant mode.
Loading status checks…
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 dotnet/aspnet-build May 14, 2021
@dougbu dougbu requested review from BrennanConroy, halter73 and dotnet/aspnet-blazor-eng as code owners May 14, 2021
@dougbu
Copy link
Contributor Author

@dougbu dougbu commented May 14, 2021 •
edited

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 reviewed May 14, 2021
View 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>

This comment has been minimized.

Sign in to view
@dougbu

dougbu May 14, 2021
Author Contributor

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

This comment has been minimized.

Sign in to view
@dougbu

dougbu May 14, 2021
Author Contributor

@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❔
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)" />

This comment has been minimized.

Sign in to view
@dougbu

dougbu May 14, 2021
Author Contributor

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 dougbu commented May 14, 2021 •
edited

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 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 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.
@HaoK
HaoK approved these changes May 14, 2021
View changes
@dougbu dougbu merged commit c53700b into main May 14, 2021
26 checks passed
26 checks passed
@net-helix
Build Analysis Build Result Analysis
Details
@azure-pipelines
aspnetcore-ci Build #20210513.38 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Build: Linux ARM) Build Build: Linux ARM succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Build: Linux ARM64) Build Build: Linux ARM64 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Build: Linux Musl ARM) Build Build: Linux Musl ARM succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Build: Linux Musl ARM64) Build Build: Linux Musl ARM64 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Build: Linux Musl x64) Build Build: Linux Musl x64 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Build: Linux x64) Build Build: Linux x64 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Build: Windows ARM) Build Build: Windows ARM succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Build: Windows ARM64) Build Build: Windows ARM64 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Build: Windows x64/x86) Build Build: Windows x64/x86 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Build: macOS arm64) Build Build: macOS arm64 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Build: macOS x64) Build Build: macOS x64 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Code check) Build Code check succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Test: Linux Source Build) Build Test: Linux Source Build succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Test: Templates - Windows Server 2016 x64) Build Test: Templates - Windows Server 2016 x64 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Test: Ubuntu 18.04 x64) Build Test: Ubuntu 18.04 x64 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Test: Windows Server 2016 x64) Build Test: Windows Server 2016 x64 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Test: macOS 10.14) Build Test: macOS 10.14 succeeded
Details
@azure-pipelines
aspnetcore-ci (Build Tests: Helix x64) Build Tests: Helix x64 succeeded
Details
@azure-pipelines
aspnetcore-quarantined-pr Build #20210513.38 had test failures
Details
@azure-pipelines
aspnetcore-quarantined-pr (Tests: Helix) Tests: Helix succeeded with issues
Details
@azure-pipelines
aspnetcore-quarantined-pr (Tests: Ubuntu 16.04 x64) Tests: Ubuntu 16.04 x64 succeeded
Details
@azure-pipelines
aspnetcore-quarantined-pr (Tests: Windows x64) Tests: Windows x64 succeeded with issues
Details
@azure-pipelines
aspnetcore-quarantined-pr (Tests: macOS 10.14) Tests: macOS 10.14 succeeded with issues
Details
license/cla All CLA requirements met.
Details
@dougbu dougbu deleted the dougbu/signalarggh branch May 14, 2021
@msftbot msftbot bot added this to the 6.0-preview5 milestone May 14, 2021
@SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented May 14, 2021

Thanks @dougbu and @captainsafia for resolving this!
@BrennanConroy
BrennanConroy reviewed May 14, 2021
View 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>

This comment has been minimized.

Sign in to view
@BrennanConroy

BrennanConroy May 14, 2021
Contributor

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

This comment has been minimized.

Sign in to view
@wtgodbe

wtgodbe May 14, 2021
Member

@dougbu I'm also wondering about this

This comment has been minimized.

Sign in to view
@dougbu

dougbu May 14, 2021
Author Contributor

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

This comment has been minimized.

Sign in to view
@dougbu

dougbu May 14, 2021
Author Contributor

Fixed in #32680
dougbu added a commit that referenced this pull request May 14, 2021
@dougbu
Remove inner <RepoRoot/> element
Verified
This commit was signed with the committer’s verified signature.
dougbu Doug Bunting
GPG key ID: EE41520987982C03 Learn about vigilant mode.
Loading status checks…
7f0f6fa 
- typo introduced in #32664
- see discussion at #32664 (comment)
@dougbu dougbu mentioned this pull request May 14, 2021
Merged
msftbot bot pushed a commit that referenced this pull request May 14, 2021
@dougbu
Remove inner <RepoRoot/> element (#32680)
Verified
This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: 4AEE18F83AFDEB23 Learn about vigilant mode.
Loading status checks…
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

@BrennanConroy BrennanConroy

@wtgodbe wtgodbe

@HaoK HaoK

@SteveSandersonMS SteveSandersonMS

@captainsafia captainsafia

@dotnet/aspnet-build aspnet-build

@halter73 halter73

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

Successfully merging this pull request may close these issues.

None yet

5 participants
@dougbu @SteveSandersonMS @HaoK @BrennanConroy @wtgodbe