-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Adding accepted SDK name match pattern to SDK manifests. #7597
Adding accepted SDK name match pattern to SDK manifests. #7597
Conversation
5855d66
to
330e3c6
Compare
9a51caf
to
0f3fa49
Compare
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
c26cde0
to
1f894c1
Compare
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.
Overall looking very nice. This is mostly style/perf nits.
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs
Outdated
Show resolved
Hide resolved
} | ||
catch (RegexMatchTimeoutException ex) | ||
{ | ||
ErrorUtilities.ThrowInternalError("Regular expression parsing exceeds timeout for manifest {0}. Error message: {1}", manifest.Name, ex.Message); |
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 doesn't feel like an internal error to me.
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 was also torn but the only real alternative is InvalidProjectFileException
and since this is a problem with the SDK resolver I think InternalError
is a better choice.
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 agree Internal > InvalidProjectFile, but why are those the only options? Internal makes it sound like it's a bug in MSBuild. It might be a bug in one of our SDK resolvers, but it might not be, and in fact, since we've tested ours, it presumably isn't. It's a fix-your-own-build problem.
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.
Well, it is not fix-your-own-build, rather fix-your-own-msbuild-cause-you-broke-it. I do not know a way to do that out of the project files. There is an option to specify additional folder for sdk resolvers and where customer's resolvers could be in theory located, but it is my understanding that it is a non-documented env variables.
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs
Outdated
Show resolved
Hide resolved
} | ||
catch (RegexMatchTimeoutException ex) | ||
{ | ||
ErrorUtilities.ThrowInternalError("Regular expression parsing exceeds timeout for manifest {0}. Error message: {1}", manifest.Name, ex.Message); |
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 was also torn but the only real alternative is InvalidProjectFileException
and since this is a problem with the SDK resolver I think InternalError
is a better choice.
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.
First pass, everything's looking good 👍
Going to give it another pass, but don't block on it if things move smoothly.
Fixes #7293
Context
Previously all SDK resolvers were loaded and then ordered by priority. The resolvers are tried one after one until one of them succeeds. In order to decrease the number of assemblies to be load we change the behavior since ChangeWave 17.4. Now the resolvers might specify the name pattern for sdks they intend to resolve. If sdk name does not match, we would not unnecessary load the resolver.
Changes Made
This change of algorithm might lead to a change in sdk resolution in the following situation:
Then before the change the project is resolved by resolver A and after with resolver B.
Testing