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.

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

Prepare for 5.3.0-preview1 #401

Merged
merged 3 commits into from
Mar 16, 2023
Merged

Prepare for 5.3.0-preview1 #401

merged 3 commits into from
Mar 16, 2023

Conversation

wtgodbe
Copy link
Contributor

@wtgodbe wtgodbe commented Mar 15, 2023

I didn't find anything in this repo that seems to correspond to package version label (servicing, rtm, preview, etc) - I believe that's all on the TFS side since that's where we do packing. @dougbu can you confirm?

@wtgodbe wtgodbe requested review from dougbu and mkArtakMSFT March 15, 2023 17:53
Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

This is insufficient because it doesn't bump the System.Net.Http.Formatting.dll assembly version. Main change likely in src/CommonAssemblyInfo.cs

@dougbu
Copy link
Member

dougbu commented Mar 15, 2023

I didn't find anything in this repo that seems to correspond to package version label (servicing, rtm, preview, etc) - I believe that's all on the TFS side since that's where we do packing. @dougbu can you confirm?

Probably worth updating https://github.com/aspnet/AspNetWebStack/blob/main/src/WebApiHelpPage/WebApiHelpPage.nuspec and https://github.com/aspnet/AspNetWebStack/blob/main/src/WebApiHelpPage/VB/WebApiHelpPage.VB.nuspec. Not great we ignored those files since 5.1.0-alpha-0, even though the inconsistency is mainly about confusion w.r.t. the packages eventually shipped.

@dougbu
Copy link
Member

dougbu commented Mar 15, 2023

Note as well that those labels don't show up in assembly versions

@wtgodbe
Copy link
Contributor Author

wtgodbe commented Mar 15, 2023

This is insufficient because it doesn't bump the System.Net.Http.Formatting.dll assembly version. Main change likely in src/CommonAssemblyInfo.cs

I already changed src/CommonAssemblyInfo.cs - I don't see anything in the System.Net.Http.Formatting folder looks like it's setting an AssemblyVersion, am I missing something?

Probably worth updating https://github.com/aspnet/AspNetWebStack/blob/main/src/WebApiHelpPage/WebApiHelpPage.nuspec and https://github.com/aspnet/AspNetWebStack/blob/main/src/WebApiHelpPage/VB/WebApiHelpPage.VB.nuspec. Not great we ignored those files since 5.1.0-alpha-0, even though the inconsistency is mainly about confusion w.r.t. the packages eventually shipped.

Those versions haven't been touched in 10 years - are you sure we should be updating them now?

@dougbu
Copy link
Member

dougbu commented Mar 15, 2023

I already changed src/CommonAssemblyInfo.cs - I don't see anything in the System.Net.Http.Formatting folder looks like it's setting an AssemblyVersion, am I missing something?

Note CommonAssemblyInfo.cs keys off defines like ASPNETWEBPAGES to choose between different assembly versions. Need a new define for the three System.Net.Http.Formatting*.csproj files and a specific 6.0.0.0 in CommonAssemblyInfo.cs. Will also need to adjust the "more than one of" and "must define" error text.

I believe the same defines are used when building from the TFS (Tooling) repo.

are you sure we should be updating them now?

Yes, it was an oversight I noticed ages ago but kept forgetting. We just redid whatever we'd done before w/o stepping back enough to see what we were missing. Even if the versions were wildly incorrect, we're reducing confusion / inconsistency here.

@wtgodbe
Copy link
Contributor Author

wtgodbe commented Mar 15, 2023

Note CommonAssemblyInfo.cs keys off defines like ASPNETWEBPAGES to choose between different assembly versions. Need a new define for the three System.Net.Http.Formatting*.csproj files and a specific 6.0.0.0 in CommonAssemblyInfo.cs. Will also need to adjust the "more than one of" and "must define" error text.

The three System.Net.Http.Formatting projects already define ASPNETMVC - are you saying they should instead define something like ASPNETHTTPFORMATTING which gets 5.3.0.0, while ASPNETMVC stays at 5.2.9.0?

and a specific 6.0.0.0 in CommonAssemblyInfo.cs

It's not clear to me what you mean by this, could you elaborate? What should that apply to?

Yes, it was an oversight I noticed ages ago but kept forgetting. We just redid whatever we'd done before w/o stepping back enough to see what we were missing. Even if the versions were wildly incorrect, we're reducing confusion / inconsistency here.

Ok, I'll update that with my next commit

@dougbu
Copy link
Member

dougbu commented Mar 15, 2023

Note CommonAssemblyInfo.cs keys off defines like ASPNETWEBPAGES to choose between different assembly versions. Need a new define for the three System.Net.Http.Formatting*.csproj files and a specific 6.0.0.0 in CommonAssemblyInfo.cs. Will also need to adjust the "more than one of" and "must define" error text.

The three System.Net.Http.Formatting projects already define ASPNETMVC - are you saying they should instead define something like ASPNETHTTPFORMATTING which gets 5.3.0.0, while ASPNETMVC stays at 5.2.9.0?

Sort of. ASPNETHTTPFORMATTING sounds fine but that should use 6.0.0.0 (a major version bump because those assemblies are changing a lot) while ASPNETMVC just changes its minor version i.e, moves from 5.2.9.0 to 5.3.0.0.

and a specific 6.0.0.0 in CommonAssemblyInfo.cs

It's not clear to me what you mean by this, could you elaborate? What should that apply to?

Sorry. I was just trying to separate adding the define (in the projects) from using it (in the C# file).

@wtgodbe
Copy link
Contributor Author

wtgodbe commented Mar 16, 2023

Updated

@@ -24,12 +24,17 @@
// BUILD_GENERATED_VERSION will be set in any CI build. Versions below are not used.
// ===================================================================================

#if (ASPNETMVC && (ASPNETWEBPAGES || ASPNETFACEBOOK)) || (ASPNETWEBPAGES && ASPNETFACEBOOK)
#error Runtime projects cannot define more than one of ASPNETMVC, ASPNETWEBPAGES or ASPNETFACEBOOK
#if (ASPNETMVC && (ASPNETWEBPAGES || ASPNETFACEBOOK || ASPNETHTTPFORMATTING)) || (ASPNETWEBPAGES && (ASPNETFACEBOOK || ASPNETHTTPFORMATTING)) || (ASPNETFACEBOOK && ASPNETHTTPFORMATTING)
Copy link
Member

Choose a reason for hiding this comment

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

😁

@wtgodbe wtgodbe merged commit 1231b77 into main Mar 16, 2023
@wtgodbe wtgodbe deleted the wtgodbe/5.3.0 branch March 16, 2023 19:56
@mkArtakMSFT mkArtakMSFT added this to the 3.3.0 (5.3.0) milestone Apr 3, 2023
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.

3 participants