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

Resolve WindowsDesktop and AspNetCore tests correctly #1285

Merged
merged 2 commits into from
Jan 4, 2023

Conversation

CharliePoole
Copy link
Collaborator

@CharliePoole CharliePoole commented Jan 1, 2023

Fixes #1274

As noted, this fix backs out the changes, which resulted in issue #1274 and adds code to the resolver to directly locate the two additional frameworks, Microsoft.WindowsDesktop.App and Microsoft.AspNetCore.App, when the test assembly depends on one of them.

This is a major change and should have a code review if someone has the time. I'll leave it for a few days before merging.

@mikkelbu
Copy link
Member

mikkelbu commented Jan 3, 2023

I'll take a look now

Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

I don't know much about this area of the codebase, but as far as I can tell the changes look sound. My main worry is whether the regkey always can be used (in windows) and whether the path is always the correct one in Linux (or we should look it up - using e.g. which dotnet or whereis dotnet).

@CharliePoole
Copy link
Collaborator Author

@mikkelbu Thanks for reviewing. A general answer to your general comment... I'm too suspect that non-standard installs could break this, particularly on Linux. However it wouldn't be a new issue - the same key and linux path is what we already use to figure out what versions of the runtime are installed.

In general, I think non-standard install locations are intended to be hidden from general use. This is similar to when you install a tool to a special path... dotnet cli can't find the tool unless you tell it the path in some way.

@CharliePoole CharliePoole merged commit ef2055c into version3 Jan 4, 2023
@CharliePoole CharliePoole deleted the issue-1274 branch January 4, 2023 00:10
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.

2 participants