Skip to content

Prepare for 5.3.0-preview1#401

Merged
wtgodbe merged 3 commits intomainfrom
wtgodbe/5.3.0
Mar 16, 2023
Merged

Prepare for 5.3.0-preview1#401
wtgodbe merged 3 commits intomainfrom
wtgodbe/5.3.0

Conversation

@wtgodbe
Copy link
Copy Markdown
Collaborator

@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
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

dougbu commented Mar 15, 2023

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

@wtgodbe
Copy link
Copy Markdown
Collaborator 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
Copy Markdown
Contributor

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
Copy Markdown
Collaborator 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
Copy Markdown
Contributor

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
Copy Markdown
Collaborator Author

wtgodbe commented Mar 16, 2023

Updated


#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
Copy Markdown
Contributor

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