-
Notifications
You must be signed in to change notification settings - Fork 87
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
Update condition for max version limitation & add package tags #265
Update condition for max version limitation & add package tags #265
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing the max-version limitation? That sounds like a pit of failure to me.
/// This variable enables the removal of the existing restriction on querying installed Visual Studio instances | ||
/// with a runtime version lower than or equal to the one in the current environment. | ||
/// </summary> | ||
public bool AllowQueryAllRuntimeVersions { get; set; } = Environment.GetEnvironmentVariable("DOTNET_MSBUILD_QUERY_ALL_RUNTIMES") == "1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think given the name "AllRuntimes" this should consider allowing .NET Core Locator to find VS installations, which some folks want--for instance #152.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@baronfel is there a good scenario for the inverse--.NET Framework locator finding SDK installations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be part of the bucket of scenarios where the user is almost certainly not going to be able to load the MSBuild, but they might just want to list/shell-invoke/etc the MSBuild? It could be useful, but a) wouldn't be as high of a priority in my mind, and b) would need to be behind whatever 'I promise I know this won't work if I try to in-proc this' flag/gate we've set up here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nits but looking good IMO. Can you give a minor-version bump in version.json
since this is a feature addition?
Co-authored-by: Rainer Sigwald <[email protected]>
…ure_version_restriction
/// <summary> | ||
/// Allow discovery of .NET SDK versions that are unlikely to be successfully loaded in the current process. | ||
/// </summary> | ||
/// <remarks> | ||
/// Defaults to <see langword="false"/>. Set this to <see langword="true"/> only if your application has special logic to handle loading an incompatible SDK, such as launching a new process with the target SDK's runtime. | ||
/// </remarks. | ||
public static bool AllowQueryAllRuntimeVersions { get; set; } = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I only noticed this PR now, but @YuliiaKovalova should a flag like this have been added to the VisualStudioInstanceQueryOptions rather than being a global static? Otherwise it's a bit unclear what a library like Roslyn's MSBuildWorkspace should do here since technically we're just a library and won't ever be registering MSBuild in the calling applications' process at this point -- do we set this and set it back to the old value, and pray there wasn't another thread doing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I completely missed that move! I agree with @jasonmalinowski that this made more sense on VisualStudioInstanceQueryOptions
.
Fixes
Updates restriction on the max version detection - previously it was capped by the environment version + add package metadata #101