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

Update condition for max version limitation & add package tags #265

Conversation

YuliiaKovalova
Copy link
Contributor

@YuliiaKovalova YuliiaKovalova commented Dec 14, 2023

Fixes

Updates restriction on the max version detection - previously it was capped by the environment version + add package metadata #101

Copy link
Member

@rainersigwald rainersigwald left a 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.

@YuliiaKovalova YuliiaKovalova marked this pull request as draft December 14, 2023 15:47
@YuliiaKovalova YuliiaKovalova marked this pull request as ready for review January 18, 2024 10:43
@YuliiaKovalova YuliiaKovalova changed the title Remove the max version limitation & add package tags Update condition for max version limitation & add package tags Jan 26, 2024
/// 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";
Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

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.

src/MSBuildLocator/VisualStudioInstanceQueryOptions.cs Outdated Show resolved Hide resolved
Copy link
Member

@rainersigwald rainersigwald left a 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?

src/MSBuildLocator/MSBuildLocator.cs Outdated Show resolved Hide resolved
src/MSBuildLocator/DotNetSdkLocationHelper.cs Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova merged commit 922d78d into microsoft:main Feb 5, 2024
2 checks passed
Comment on lines +46 to +52
/// <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;
Copy link
Member

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?

Copy link
Member

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.

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.

None yet

4 participants