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

Correct $(SharedFxVersion) and $(TargetingPackVersion) values #25790

Merged
merged 2 commits into from
Sep 12, 2020
Merged

Correct $(SharedFxVersion) and $(TargetingPackVersion) values #25790

merged 2 commits into from
Sep 12, 2020

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Sep 10, 2020

  • Ensure $(SharedFxVersion) doesn't change in $(NoSemVer20) projects
  • Ignore current project's $(VersionSuffix) in $(TargetingPackVersion)
    • Never assume $(AspNetCoreBaselineVersion) matches released targeting pack
  • Stabilize both versions correctly
  • Use these properties more widely
    • Remove other mechanisms to get the same values
    • Reduce use of the _GetPackageVersionInfo target
    • Reduce use of $(SharedFxVersion) for the targeting pack

nits:

  • Correct comments about old RTMVersions.csproj project
  • Fix or remove a few other comments

@dougbu dougbu requested review from JamesNK, HaoK and a team September 10, 2020 22:19
@dougbu
Copy link
Member Author

dougbu commented Sep 10, 2020

This is more work coming out of our servicing readiness exercise. My previous changes to use $(SharedFxVersion) more in our Helix setup were useful but didn't go quite far enough. This got those builds much further❕ (Then I needed to back-port too many build reliability issues ☹️)

@dougbu
Copy link
Member Author

dougbu commented Sep 10, 2020

@Pilchie any objection to tell-mode for RC2❔

HaoK
HaoK reviewed Sep 10, 2020
View reviewed changes
eng/targets/Helix.targets
<ItemGroup Condition=" '$(TestDependsOnAspNetRef)' == 'true' AND '$(IsTargetingPackBuilding)' == 'true' ">
<HelixContent Include="$(RepoRoot)artifacts\packages\Release\Shipping\Microsoft.AspNetCore.App.Ref.$(SharedFxVersion).nupkg" />
<HelixContent Include="$(RepoRoot)artifacts\packages\Release\Shipping\Microsoft.AspNetCore.App.Ref.$(TargetingPackVersion).nupkg" />
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can we put this in a variable to use in both here and the nupkg file we pass below into the commands?
Microsoft.AspNetCore.App.Ref.$(TargetingPackVersion).nupkg

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like I need to rebase anyhow. So, sure, I'll make another update here.

But, I'll go w/ your earlier suggestion of removing the *.nupkg names from the command line and instead use the passed $(SharedFxVersion) to craft the names on the Helix agent.

Copy link
Member

Choose a reason for hiding this comment

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

Nice, a few less arguments is much better since its already getting out of control with how many we pass!

@Pilchie
Copy link
Member

Pilchie commented Sep 10, 2020

Tell mode is fine with me.

@dougbu dougbu added area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework tell-mode Indicates a PR which is being merged during tell-mode labels Sep 10, 2020
@JamesNK
Copy link
Member

JamesNK commented Sep 10, 2020 •
edited
Loading

Hmmm, this PR changes versions used by the gRPC interop tests website - https://github.com/dotnet/aspnetcore/pull/25790/files#diff-016fa755746abada907d870814dee7c8

I wonder if this might be the cause of gRPC interop tests failing in my PR - #25587

Offline discussion with @JunTaoLuo:

I'm trying to update the gRPC template with a new package version for RC2. It uses new fields added to HeaderNames and the interop tests are failing because they not in the aspnetcore runtime that it is using. This is blocking me from merging for RC2.

The test writes assembly versions in the logs and this is what the server reports:

NetCoreAppVersion: 5.0.0-rc.1.20371.13+d8cf13e0ba9b369a15a83472b6b97463c6d07fe2
AspNetCoreAppVersion: 5.0.0-rc.1.20379.2

Oddly, the client writes a different version:

NetCoreAppVersion: 5.0.0-rc.2.20458.14+482494f9ecc9a630599278b80893841462c90a5b

It looks like the server is using a wrong version. It reports RC1 and this PR is targeting the RC2 branch.

Possible fix John added to that PR - 29a835f (#25587)

Could this issue be the cause instead?

@dougbu
Copy link
Member Author

dougbu commented Sep 10, 2020

Could this issue be the cause instead?

It's possible but somewhat unlikely. The two property values I'm setting here were wrong for a small subset of the projects and mainly when we're using stable versions. We're not using stable versions (outside a servicing readiness exercise) for 5.0 yet. So, this PR should have minimal impact -- for now.

The main exception is if we needed the shared Fx or targeting pack version in a project using SemVer 1.0. So far, we're 🆗 there.

So, @JunTaoLuo's fix in your PR is on the right path. Any place we made assumptions that dotnet/runtime and dotnet/aspnetcore sharedFx or TFM and dotnet/aspnetcore sharedFx versions align is usually wrong.

@dougbu dougbu force-pushed the dougbu/more.versioning.5.0 branch 4 times, most recently from 958eccb to 55eb953 Compare September 11, 2020 19:05
@dougbu
Correct $(SharedFxVersion) and $(TargetingPackVersion) values
75632bb 
- Ensure `$(SharedFxVersion)` doesn't change in `$(NoSemVer20)` projects
- Ignore current project's `$(VersionSuffix)` in `$(TargetingPackVersion)`
    - Never assume `$(AspNetCoreBaselineVersion)` matches released targeting pack
- Stabilize both versions correctly
- Use these properties more widely
    - Remove other mechanisms to get the same values
    - Reduce use of the `_GetPackageVersionInfo` target
    - Reduce use of `$(SharedFxVersion)` for the targeting pack

nits:
- Correct comments about old RTMVersions.csproj project
- Fix or remove a few other comments
@dougbu
Do not pass package filenames on Helix command Lines
76ce84d 
- remove parsing of these command-line arguments from `RuntestOptions`
  - instead craft the names using passed `$(SharedFxVersion)`
- restore `$(DotNetRuntimeSourceFeedKey)` on Helix command line
  - lost somewhere along the line
- correct argument count in runtests.sh
  - treated 11th argument as both Helix timeout and feed credential
  - count was messed up somewhere alone the line

nits:
- update C# syntax in `RuntestOptions` e.g. remove unused `public` setters
- sort and group properties and their assignments
@dougbu dougbu force-pushed the dougbu/more.versioning.5.0 branch from 55eb953 to 76ce84d Compare September 11, 2020 23:33
@dougbu
Copy link
Member Author

dougbu commented Sep 11, 2020

@HaoK @wtgodbe please have another look. The second commit changes things from what you saw when commenting.

@Pilchie any objection if I get this in tonight or over the weekend❔

@ghost
Copy link

ghost commented Sep 12, 2020

Hello @dougbu!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost
Copy link

ghost commented Sep 12, 2020

Apologies, while this PR appears ready to be merged, it looks like release/5.0-rc2 is a protected branch and I have not been granted permission to perform the merge.

1 similar comment
@ghost
Copy link

ghost commented Sep 12, 2020

Apologies, while this PR appears ready to be merged, it looks like release/5.0-rc2 is a protected branch and I have not been granted permission to perform the merge.

@dougbu dougbu merged commit 326507b into release/5.0-rc2 Sep 12, 2020
@dougbu dougbu deleted the dougbu/more.versioning.5.0 branch September 12, 2020 02:17
@Pilchie
Copy link
Member

Pilchie commented Sep 12, 2020

No objection!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wtgodbe wtgodbe wtgodbe approved these changes

@HaoK HaoK HaoK approved these changes

@JamesNK JamesNK Awaiting requested review from JamesNK

Assignees
No one assigned
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dougbu @Pilchie @JamesNK @HaoK @wtgodbe