Skip to content

Commit c24c943

Browse files
Copilotbaronfel
Copilot
and
baronfel
committed
Fix SDK environment variable precedence to override ambient env vars
Co-authored-by: baronfel <[email protected]>
1 parent 8c38947 commit c24c943

File tree

18 files changed

+220
-12
lines changed

18 files changed

+220
-12
lines changed

‎src/Build.UnitTests/Evaluation/SdkResultEvaluation_Tests.cs‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -549,6 +549,121 @@ public void SdkResolverCanSetEnvVarsThatInfluenceBuild()
549549
instance.GetItems("ExecThingsAsEnvironment").ShouldHaveSingleItem().EvaluatedInclude.ShouldBe("TestEnvVarValue");
550550
}
551551

552+
[Fact]
553+
public void SdkResolverEnvironmentVariablesOverrideAmbientEnvironmentVariables()
554+
{
555+
string originalValue = Environment.GetEnvironmentVariable("SDK_TEST_VAR");
556+
try
557+
{
558+
// Set an ambient environment variable
559+
Environment.SetEnvironmentVariable("SDK_TEST_VAR", "AmbientValue");
560+
561+
var projectOptions = SdkUtilities.CreateProjectOptionsWithResolver(new SdkUtilities.ConfigurableMockSdkResolver((_, _, factory) =>
562+
factory.IndicateSuccess(Path.Combine(_testFolder, "Sdk"), "1.0.0", null, null, null, environmentVariablesToAdd: new Dictionary<string, string>
563+
{
564+
{ "SDK_TEST_VAR", "SdkValue" }
565+
})));
566+
567+
string projectContent = """
568+
<Project Sdk="sdkenvvar">
569+
<PropertyGroup>
570+
<CapturedValue>$(SDK_TEST_VAR)</CapturedValue>
571+
</PropertyGroup>
572+
<Target Name="TestTarget">
573+
<PropertyGroup>
574+
<ExecValue>$(SDK_TEST_VAR)</ExecValue>
575+
</PropertyGroup>
576+
</Target>
577+
</Project>
578+
""";
579+
580+
string projectPath = Path.Combine(_testFolder, "project.proj");
581+
File.WriteAllText(projectPath, projectContent);
582+
583+
string sdkPropsContents = "<Project />";
584+
string sdkPropsPath = Path.Combine(_testFolder, "Sdk", "Sdk.props");
585+
Directory.CreateDirectory(Path.Combine(_testFolder, "Sdk"));
586+
File.WriteAllText(sdkPropsPath, sdkPropsContents);
587+
588+
string sdkTargetsContents = @"<Project />";
589+
string sdkTargetsPath = Path.Combine(_testFolder, "Sdk", "Sdk.targets");
590+
File.WriteAllText(sdkTargetsPath, sdkTargetsContents);
591+
592+
var project = CreateProject(projectPath, projectOptions);
593+
var instance = project.CreateProjectInstance();
594+
595+
_logger.AssertNoErrors();
596+
_logger.AssertNoWarnings();
597+
598+
// The SDK value should override the ambient environment variable
599+
instance.GetPropertyValue("CapturedValue").ShouldBe("SdkValue");
600+
601+
instance.Build(["TestTarget"], [_logger], out var targetOutputs);
602+
603+
instance.GetPropertyValue("ExecValue").ShouldBe("SdkValue");
604+
}
605+
finally
606+
{
607+
Environment.SetEnvironmentVariable("SDK_TEST_VAR", originalValue);
608+
}
609+
}
610+
611+
[Fact]
612+
public void FirstSdkEnvironmentVariableWinsOverSubsequentSdks()
613+
{
614+
var projectOptions = SdkUtilities.CreateProjectOptionsWithResolver(new SdkUtilities.ConfigurableMockSdkResolver((sdkReference, _, factory) =>
615+
{
616+
// First SDK sets the variable
617+
if (sdkReference.Name == "FirstSdk")
618+
{
619+
return factory.IndicateSuccess(Path.Combine(_testFolder, "Sdk1"), "1.0.0", null, null, null, environmentVariablesToAdd: new Dictionary<string, string>
620+
{
621+
{ "SHARED_VAR", "FirstValue" }
622+
});
623+
}
624+
// Second SDK tries to set it, but should be ignored
625+
else if (sdkReference.Name == "SecondSdk")
626+
{
627+
return factory.IndicateSuccess(Path.Combine(_testFolder, "Sdk2"), "1.0.0", null, null, null, environmentVariablesToAdd: new Dictionary<string, string>
628+
{
629+
{ "SHARED_VAR", "SecondValue" }
630+
});
631+
}
632+
return null;
633+
}));
634+
635+
string projectContent = """
636+
<Project>
637+
<Import Project="Sdk.props" Sdk="FirstSdk"/>
638+
<Import Project="Sdk.props" Sdk="SecondSdk"/>
639+
<PropertyGroup>
640+
<CapturedValue>$(SHARED_VAR)</CapturedValue>
641+
</PropertyGroup>
642+
</Project>
643+
""";
644+
645+
string projectPath = Path.Combine(_testFolder, "project.proj");
646+
File.WriteAllText(projectPath, projectContent);
647+
648+
string sdk1PropsContents = "<Project />";
649+
string sdk1PropsPath = Path.Combine(_testFolder, "Sdk1", "Sdk.props");
650+
Directory.CreateDirectory(Path.Combine(_testFolder, "Sdk1"));
651+
File.WriteAllText(sdk1PropsPath, sdk1PropsContents);
652+
653+
string sdk2PropsContents = "<Project />";
654+
string sdk2PropsPath = Path.Combine(_testFolder, "Sdk2", "Sdk.props");
655+
Directory.CreateDirectory(Path.Combine(_testFolder, "Sdk2"));
656+
File.WriteAllText(sdk2PropsPath, sdk2PropsContents);
657+
658+
var project = CreateProject(projectPath, projectOptions);
659+
660+
_logger.AssertNoErrors();
661+
_logger.AssertNoWarnings();
662+
663+
// The first SDK's value should win
664+
project.GetPropertyValue("CapturedValue").ShouldBe("FirstValue");
665+
}
666+
552667
public void Dispose()
553668
{
554669
_env.Dispose();

‎src/Build/BackEnd/Components/SdkResolution/SdkResult.cs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ public void Translate(ITranslator translator)
9292
keyTranslator: (ITranslator t, ref string s) => t.Translate(ref s),
9393
valueTranslator: SdkResultTranslationHelpers.Translate,
9494
dictionaryCreator: count => new Dictionary<string, SdkResultItem>(count, StringComparer.OrdinalIgnoreCase));
95+
translator.TranslateDictionary(ref _environmentVariablesToAdd, count => new Dictionary<string, string>(count, StringComparer.OrdinalIgnoreCase));
9596

9697
translator.Translate(ref _sdkReference);
9798
}
@@ -112,6 +113,7 @@ public override bool Equals(object obj)
112113
_additionalPaths?.Count == result._additionalPaths?.Count &&
113114
_propertiesToAdd?.Count == result._propertiesToAdd?.Count &&
114115
_itemsToAdd?.Count == result._propertiesToAdd?.Count &&
116+
_environmentVariablesToAdd?.Count == result._environmentVariablesToAdd?.Count &&
115117
EqualityComparer<SdkReference>.Default.Equals(_sdkReference, result._sdkReference))
116118
{
117119
if (_additionalPaths != null)
@@ -147,6 +149,17 @@ public override bool Equals(object obj)
147149
}
148150
}
149151

152+
if (_environmentVariablesToAdd != null)
153+
{
154+
foreach (var envVarToAdd in _environmentVariablesToAdd)
155+
{
156+
if (result._environmentVariablesToAdd[envVarToAdd.Key] != envVarToAdd.Value)
157+
{
158+
return false;
159+
}
160+
}
161+
}
162+
150163
return true;
151164
}
152165

@@ -185,6 +198,14 @@ public override int GetHashCode()
185198
hashCode = (hashCode * -1521134295) + itemToAdd.Value.GetHashCode();
186199
}
187200
}
201+
if (_environmentVariablesToAdd != null)
202+
{
203+
foreach (var envVarToAdd in _environmentVariablesToAdd)
204+
{
205+
hashCode = (hashCode * -1521134295) + envVarToAdd.Key.GetHashCode();
206+
hashCode = (hashCode * -1521134295) + envVarToAdd.Value.GetHashCode();
207+
}
208+
}
188209

189210
return hashCode;
190211
}

‎src/Build/Definition/Project.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4452,9 +4452,8 @@ public IItemDefinition<ProjectMetadata> GetItemDefinition(string itemType)
44524452
/// <param name="value">Environment variable value.</param>
44534453
public void AddSdkResolvedEnvironmentVariable(string name, string value)
44544454
{
4455-
// If the property has already been set as an environment variable or by another SDK, we do not overwrite it.
4456-
if (EnvironmentVariablePropertiesDictionary?.Contains(name) == true
4457-
|| SdkResolvedEnvironmentVariablePropertiesDictionary?.Contains(name) == true)
4455+
// If another SDK already set it, we do not overwrite it.
4456+
if (SdkResolvedEnvironmentVariablePropertiesDictionary?.Contains(name) == true)
44584457
{
44594458
return;
44604459
}
@@ -4464,6 +4463,7 @@ public void AddSdkResolvedEnvironmentVariable(string name, string value)
44644463
SdkResolvedEnvironmentVariablePropertiesDictionary ??= new();
44654464
SdkResolvedEnvironmentVariablePropertiesDictionary.Set(property);
44664465

4466+
// SDK-resolved environment variables override ambient environment variables.
44674467
SetProperty(name, value, isGlobalProperty: false, mayBeReserved: false, loggingContext: null);
44684468
}
44694469

‎src/Build/Instance/ProjectInstance.cs‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1387,14 +1387,8 @@ PropertyDictionary<ProjectPropertyInstance> IEvaluatorData<ProjectPropertyInstan
13871387

13881388
public void AddSdkResolvedEnvironmentVariable(string name, string value)
13891389
{
1390-
// If the property has already been set as an environment variable, we do not overwrite it.
1391-
if (_environmentVariableProperties.Contains(name))
1392-
{
1393-
_loggingContext.LogComment(MessageImportance.Low, "SdkEnvironmentVariableAlreadySet", name, value);
1394-
return;
1395-
}
13961390
// If another SDK already set it, we do not overwrite it.
1397-
else if (_sdkResolvedEnvironmentVariableProperties?.Contains(name) == true)
1391+
if (_sdkResolvedEnvironmentVariableProperties?.Contains(name) == true)
13981392
{
13991393
_loggingContext.LogComment(MessageImportance.Low, "SdkEnvironmentVariableAlreadySetBySdk", name, value);
14001394
return;
@@ -1406,8 +1400,18 @@ public void AddSdkResolvedEnvironmentVariable(string name, string value)
14061400

14071401
_sdkResolvedEnvironmentVariableProperties.Set(property);
14081402

1409-
// Only set the local property if it does not already exist, prioritizing regular properties defined in XML.
1410-
if (GetProperty(name) is null)
1403+
// SDK-resolved environment variables override ambient environment variables.
1404+
bool overridingAmbient = _environmentVariableProperties.Contains(name);
1405+
if (overridingAmbient)
1406+
{
1407+
_environmentVariableProperties.Remove(name);
1408+
_loggingContext.LogComment(MessageImportance.Low, "SdkEnvironmentVariableOverridingAmbient", name, value);
1409+
}
1410+
1411+
// Set the property, overriding ambient environment variables but not regular properties defined in XML.
1412+
// If we're overriding an ambient variable, or if the property doesn't exist, set it.
1413+
// Otherwise, prioritize regular properties defined in XML.
1414+
if (overridingAmbient || GetProperty(name) is null)
14111415
{
14121416
((IEvaluatorData<ProjectPropertyInstance, ProjectItemInstance, ProjectMetadataInstance, ProjectItemDefinitionInstance>)this)
14131417
.SetProperty(name, value, isGlobalProperty: false, mayBeReserved: false, loggingContext: _loggingContext, isEnvironmentVariable: true, isCommandLineProperty: false);

‎src/Build/Resources/Strings.resx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1392,6 +1392,9 @@ Errors: {3}</value>
13921392
<data name="SdkEnvironmentVariableSet" xml:space="preserve">
13931393
<value>An SDK attempted set the environment variable "{0}" to "{1}".</value>
13941394
</data>
1395+
<data name="SdkEnvironmentVariableOverridingAmbient" xml:space="preserve">
1396+
<value>An SDK set the environment variable "{0}" to "{1}", overriding the ambient environment variable value.</value>
1397+
</data>
13951398
<data name="UnrecognizedParentElement" xml:space="preserve">
13961399
<value>MSB4189: &lt;{1}&gt; is not a valid child of the &lt;{0}&gt; element.</value>
13971400
<comment>{StrBegin="MSB4189: "}</comment>

‎src/Build/Resources/xlf/Strings.cs.xlf‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/Build/Resources/xlf/Strings.de.xlf‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/Build/Resources/xlf/Strings.es.xlf‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/Build/Resources/xlf/Strings.fr.xlf‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/Build/Resources/xlf/Strings.it.xlf‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)