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

Avoid intermediate FullNugetVersion file #47542

Merged
merged 4 commits into from
Mar 14, 2025

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Mar 13, 2025

Use the same mechanism as in aspnetcore, windowsdesktop and runtime to retrieve the non stable product version in Publishing.props and update the Artifact items with that version.

Also remove IsShipping="true" from the artifacts as that's the default.

Use the same mechanism as in aspnetcore, windowsdesktop and
runtime to retrieve the non stable product version in Publishing.props
and update the Artifact items with that version.

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Mar 13, 2025
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

I did like the simplicity of the previous approach and how it let us avoid custom targets in Publishing.props, but I'm not going to block merging this in on that.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Mar 13, 2025

I did like the simplicity of the previous approach and how it let us avoid custom targets in Publishing.props, but I'm not going to block merging this in on that.

The current infrastructure has some flaws. The file needs to always be created, in all BuildPasses that build the sdk repo. That means that if workloads get moved into a separate job, i.e. win-arm64 BP2, that file will need to be created there as well. But the code for that lives in redist-installer.proj which doesn't get invoked in that vertical. Given that the original author who implemented this labeled it as hack and because I share that classification I submitted this PR.

I find it more robust to directly ask for the desired version than to depend on A writing state to a location and B reading from it.

That said, I think it should eventually be fine to just depend on the repo wide versioning and not even ask redist-installer.proj for the desired version. That's currently not possible but with some more refactoring, it might be.

@ViktorHofer ViktorHofer merged commit afb7ffc into main Mar 14, 2025
36 of 39 checks passed
@ViktorHofer ViktorHofer deleted the RemoveIntermeidateFullNugetVersionFile branch March 14, 2025 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants