Skip to content

Conversation

@nagilson
Copy link
Member

@nagilson nagilson commented Oct 21, 2025

  • Adds validation using hostfxr and layout expectations to ensure the install is valid
  • Refactors release manifest parsing to greatly simplify version parsing. Fixes bugs with incorrect version utilization.
  • Adds ability to debug into the dnup test process and attach using the environment variable DNUP_TEST_DEBUG or vscode workspace launch profile.
  • Adds tests that install into the same directory to test concurrency.

This implements validation of installs using hostfxr apis to ensure the install actually works and not just that the manifest is tracking the installs correctly in e2e tests.

It also adds a test to show that we can do multiple installs in the same directory without failing.

It also improves the existing test logic to not assume a hard-coded debug value for the dnup process.
@nagilson
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nagilson nagilson marked this pull request as ready for review October 22, 2025 21:01
@nagilson nagilson requested a review from a team as a code owner October 22, 2025 21:01
@nagilson nagilson requested a review from dsplaisted October 22, 2025 21:01
: "libhostfxr.so";

return Directory.EnumerateFiles(hostFxrDirectory, libraryName, SearchOption.AllDirectories)
.OrderByDescending(File.GetLastWriteTimeUtc)
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is the best approach at determining which hostfxr to use, though the cases where this would be called is rare. I would be ok with removing this or instead searching the latest directory


Rule ID | Missing Help Link | Title |
--------|-------------------|-------|
CA1873 | <https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1873> | Avoid potentially expensive logging |
Copy link
Member Author

Choose a reason for hiding this comment

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

This is from main - happens automatically in build

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

Looks great, big improvement to the readability / understandability of ReleaseManifest.

Something to think about for the future is I think it would be a good idea to get rid of the ParseVersionChannel logic in ReleaseManifest. Probably we can move that to the UpdateChannel class and also take advantage of logic in ReleaseVersion as possible.

@nagilson nagilson enabled auto-merge October 23, 2025 19:35
@nagilson
Copy link
Member Author

Looks great, big improvement to the readability / understandability of ReleaseManifest.

Something to think about for the future is I think it would be a good idea to get rid of the ParseVersionChannel logic in ReleaseManifest. Probably we can move that to the UpdateChannel class and also take advantage of logic in ReleaseVersion as possible.

Agreed. Caching and separating the archive download logic would also be ideal.

@nagilson nagilson merged commit 6c89278 into dotnet:dnup Oct 23, 2025
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants