Sdkresolvers can use environment variable tracking #1
Conversation
…hen using binary logger.
…ibility. Update dependencies of it.
| _loggingContext.LogBuildEvent(args); | ||
| } | ||
|
|
||
| return Environment.GetEnvironmentVariable(name) ?? string.Empty; |
There was a problem hiding this comment.
I'd favor not returning empty if the environment variable does not exist. In msbuildian an empty string is returned for non-existing env vars, but in C# null is returned.
| _loggingContext = loggingContext; | ||
| } | ||
|
|
||
| public override string GetGlobalPropertyValue(string name) |
There was a problem hiding this comment.
Why expose these to the resolver? AFAIK resolvers didn't have access to global properties before. I'd vote on keeping in that way to minimize their complexity.
There was a problem hiding this comment.
Looking at the rippling changes required to pipe global properties, I'm more convinced we shouldn't expose them.
There was a problem hiding this comment.
This is so that users can override settings that the NuGet SDK resolver uses. Right now it can only get its properties from nuget.config
There was a problem hiding this comment.
I'd separate the changes in two PRs then. Making sdk resolution depend on global properties also requires changing sdk resolver caching: #1 (comment)
| @@ -83,6 +89,7 @@ public void Translate(ITranslator translator) | |||
| translator.Translate(ref _submissionId); | |||
| translator.Translate(ref _version); | |||
| translator.Translate(ref _interactive); | |||
| translator.TranslateDictionary(ref _globalProperties, count => new Dictionary<string, string>(count, StringComparer.OrdinalIgnoreCase)); | |||
There was a problem hiding this comment.
This will make node communication slower. Each node calls back into the main node when resolving sdks.
| @@ -53,7 +55,7 @@ public override SdkResult ResolveSdk(int submissionId, SdkReference sdk, Logging | |||
| */ | |||
| result = cached.GetOrAdd( | |||
| sdk.Name, | |||
| key => base.ResolveSdk(submissionId, sdk, loggingContext, sdkReferenceLocation, solutionPath, projectPath, interactive)); | |||
| key => base.ResolveSdk(submissionId, sdk, loggingContext, sdkReferenceLocation, solutionPath, projectPath, interactive, globalProperties)); | |||
There was a problem hiding this comment.
This is probably a correctness argument for not passing global properties. Sdks are currently cached per build submission and not per build request (a build submission has multiple requests). We'd have to redo their caching to change the key to be a Microsoft.Build.BackEnd.ConfigurationMetadata. But by doing this we'd loose perf, and I don't see what we gain for it.
| @@ -52,11 +55,15 @@ public SdkResult() | |||
|
|
|||
| public override string Version => _version; | |||
|
|
|||
| public override bool TrackEnvironmentVariables { get => _trackEnvironmentVariables; set => _trackEnvironmentVariables = value; } | |||
There was a problem hiding this comment.
Seems superfluous to have resolvers dynamically respond whether they track environment variables on every sdk resolution. Probably better to ask them once, during resolver loading in Microsoft.Build.BackEnd.SdkResolution.SdkResolverLoader
Like SdkResolver.GetCapabilities, and the env tracking would be part of a return enum.
| @@ -32,7 +32,7 @@ public sealed class BinaryLogger : ILogger | |||
| // version 7: | |||
| // - Include ProjectStartedEventArgs.GlobalProperties | |||
| // version 8: | |||
| // - new record kinds: EnvironmentVariableRead, PropertyReassignment, UninitializedPropertyRead | |||
There was a problem hiding this comment.
@KirillOsenkov do you prefer a version number bump instead, to avoid a small window of time where version 8 would be broken?
There was a problem hiding this comment.
I can bump it. Will do next commit.
These changes surface global properties and environment variables to SDK Resolvers. This allows them to have their environment variable requests be tracked.
SDK Resolvers will have to acknowledge that they have opted-in to environment variable tracking on the returned SdkResult.