Skip to content

Use Microsoft.AspNetCore.InternalTesting.csproj for _GetPackageVersionInfo #53152

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

Merged
merged 5 commits into from
Jan 5, 2024

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Jan 4, 2024

There's no need to use a node project to calculate the nonstable package version of the repo during publishing - use Microsoft.AspNetCore.InternalTesting.csproj instead.

Related - #53091. @javiercn, is the commented-out portion of the target going to be used anywhere other than in publishing.props? If so we should separate that out into a separate target & name it something else.

@wtgodbe wtgodbe requested a review from a team as a code owner January 4, 2024 20:15
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jan 4, 2024
@wtgodbe wtgodbe requested a review from javiercn January 4, 2024 20:17
<PackageVersion>$(PackageVersion)</PackageVersion>
</_NodePackageNameAndVersions>
</_ResolvedPackageVersionInfo>
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

The other part is that this doesn't need to match <Output TaskParameter="TargetOutputs" ItemName="_ResolvedPackageVersionInfo" /> The TargetOuput here is provided in the Returns, ItemName is only where in the calling context the output needs to be placed, but they both don't have to match.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, then we must just not be setting PackageVersion in the node project anymore. There's no strong reason to use that project as far as I know, I'll try using dotnet-user-jwts.

@javiercn
Copy link
Member

javiercn commented Jan 4, 2024

Related - #53091. @javiercn, is the commented-out portion of the target going to be used anywhere other than in publishing.props? If so we should separate that out into a separate target & name it something else.

I was keeping the structure as close as possible to what was there. I don't feel strongly about it either way.

The previous code was grabbing the version from the functional test project directly, which was a bit random.

I don't know exactly what the version info is being used for, so I just made sure we captured all the versions. In reality we version all the packages simultaneously.

In my view, if we are generating some sort of manifest, we should include the versions for all packages that we ship, just in case we break something and produce the wrong version.

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 4, 2024

I don't know exactly what the version info is being used for, so I just made sure we captured all the versions. In reality we version all the packages simultaneously.

In my view, if we are generating some sort of manifest, we should include the versions for all packages that we ship, just in case we break something and produce the wrong version.

The comment below isn't entirely accurate:

https://github.com/dotnet/aspnetcore/blame/1c6c34733003e5236315b959d1729be2bf14968d/eng/Publishing.props#L57-L60

The target in which we call _GetPackageVersionInfo will return the PackageVersion only for the project on which we called it - then we use that package version to construct a path for publish files to be dropped in to. We chose an arbitrary nonshipping package for that, because we need a unique path per-build. We can't just use the PackageVersion property directly in publishing.props, because it hasn't been set yet by the time we get there, so we have to do it in a target. Historically the arbitrary nonshipping package we chose was an npmproj - I don't think there's any strong reason to keep trying to use the new nodeproj, hence why I suggested dotnet-user-jwts. Now that I think about it though, I think Microsoft.Aspnetcore.Testing makes more sense. I'm going to update this PR now & try a test internal build.

@javiercn
Copy link
Member

javiercn commented Jan 4, 2024

The comment below isn't entirely accurate:

https://github.com/dotnet/aspnetcore/blame/1c6c34733003e5236315b959d1729be2bf14968d/eng/Publishing.props#L57-L60

The target in which we call _GetPackageVersionInfo will return the PackageVersion only for the project on which we called it - then we use that package version to construct a path for publish files to be dropped in to. We chose an arbitrary nonshipping package for that, because we need a unique path per-build. We can't just use the PackageVersion property directly in publishing.props, because it hasn't been set yet by the time we get there, so we have to do it in a target. Historically the arbitrary nonshipping package we chose was an npmproj - I don't think there's any strong reason to keep trying to use the new nodeproj, hence why I suggested dotnet-user-jwts. Now that I think about it though, I think Microsoft.Aspnetcore.Testing makes more sense. I'm going to update this PR now & try a test internal build.

Seems that we can rip the entire thing in that case.

@wtgodbe wtgodbe changed the title Fix return value in _GetPackageVersionInfo Use Microsoft.AspNetCore.InternalTesting.csproj for _GetPackageVersionInfo Jan 4, 2024
@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 4, 2024

Test build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2345984&view=results

Seems that we can rip the entire thing in that case.

Agreed - #53153

@wtgodbe wtgodbe merged commit 2683808 into main Jan 5, 2024
@wtgodbe wtgodbe deleted the wtgodbe-patch-1 branch January 5, 2024 04:25
@ghost ghost added this to the 9.0-preview1 milestone Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants