Skip to content

Commit

Permalink
Use -e to set env variables
Browse files Browse the repository at this point in the history
  • Loading branch information
tmat committed Jan 9, 2025
1 parent 28b40df commit 83c6d4e
Show file tree
Hide file tree
Showing 10 changed files with 95 additions and 113 deletions.
11 changes: 0 additions & 11 deletions src/BuiltInTools/DotNetDeltaApplier/StartupHook.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ internal sealed class StartupHook
{
private static readonly bool s_logToStandardOutput = Environment.GetEnvironmentVariable(EnvironmentVariables.Names.HotReloadDeltaClientLogMessages) == "1";
private static readonly string s_namedPipeName = Environment.GetEnvironmentVariable(EnvironmentVariables.Names.DotnetWatchHotReloadNamedPipeName);
private static readonly string s_targetProcessPath = Environment.GetEnvironmentVariable(EnvironmentVariables.Names.DotnetWatchHotReloadTargetProcessPath);

/// <summary>
/// Invoked by the runtime when the containing assembly is listed in DOTNET_STARTUP_HOOKS.
Expand All @@ -21,16 +20,6 @@ public static void Initialize()
{
var processPath = Environment.GetCommandLineArgs().FirstOrDefault();

// Workaround for https://github.com/dotnet/sdk/issues/40484
// When launching the application process dotnet-watch sets Hot Reload environment variables via CLI environment directives (dotnet [env:X=Y] run).
// Currently, the CLI parser sets the env variables to the dotnet.exe process itself, rather then to the target process.
// This may cause the dotnet.exe process to connect to the named pipe and break it for the target process.
if (!IsMatchingProcess(processPath, s_targetProcessPath))
{
Log($"Ignoring process '{processPath}', expecting '{s_targetProcessPath}'");
return;
}

Log($"Loaded into process: {processPath}");

ClearHotReloadEnvironmentVariables();
Expand Down
62 changes: 40 additions & 22 deletions src/BuiltInTools/dotnet-watch/Aspire/AspireServiceFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ private readonly struct Session(string dcpId, string sessionId, RunningProject r

private readonly ProjectLauncher _projectLauncher;
private readonly AspireServerService _service;
private readonly IReadOnlyList<string> _buildArguments;
private readonly ProjectOptions _hostProjectOptions;

/// <summary>
/// Lock to access:
Expand All @@ -43,10 +43,10 @@ private readonly struct Session(string dcpId, string sessionId, RunningProject r
private int _sessionIdDispenser;
private volatile bool _isDisposed;

public SessionManager(ProjectLauncher projectLauncher, IReadOnlyList<string> buildArguments)
public SessionManager(ProjectLauncher projectLauncher, ProjectOptions hostProjectOptions)
{
_projectLauncher = projectLauncher;
_buildArguments = buildArguments;
_hostProjectOptions = hostProjectOptions;

_service = new AspireServerService(
this,
Expand Down Expand Up @@ -209,38 +209,56 @@ private ProjectOptions GetProjectOptions(ProjectLaunchRequest projectLaunchInfo)
{
"--project",
projectLaunchInfo.ProjectPath,
// TODO: https://github.com/dotnet/sdk/issues/43946
// Need to suppress launch profile for now, otherwise it would override the port set via env variable.
"--no-launch-profile",
};

//if (projectLaunchInfo.DisableLaunchProfile)
//{
// arguments.Add("--no-launch-profile");
//}
//else if (!string.IsNullOrEmpty(projectLaunchInfo.LaunchProfile))
//{
// arguments.Add("--launch-profile");
// arguments.Add(projectLaunchInfo.LaunchProfile);
//}
// Implements https://github.com/dotnet/aspire/blob/main/docs/specs/IDE-execution.md#launch-profile-processing-project-launch-configuration

if (projectLaunchInfo.DisableLaunchProfile)
{
arguments.Add("--no-launch-profile");
}
else if (!string.IsNullOrEmpty(projectLaunchInfo.LaunchProfile))
{
arguments.Add("--launch-profile");
arguments.Add(projectLaunchInfo.LaunchProfile);
}
else if (!_hostProjectOptions.NoLaunchProfile && _hostProjectOptions.LaunchProfileName != null)
{
arguments.Add("--launch-profile");
arguments.Add(_hostProjectOptions.LaunchProfileName);
}

if (projectLaunchInfo.Arguments != null)
{
arguments.AddRange(projectLaunchInfo.Arguments);
if (projectLaunchInfo.Arguments.Any())
{
arguments.AddRange(projectLaunchInfo.Arguments);
}
else
{
// indicate that no arguments should be used even if launch profile specifies some:
arguments.Add("--no-launch-profile-arguments");
}
}

foreach (var (name, value) in projectLaunchInfo.Environment ?? [])
{
arguments.Add("-e");
arguments.Add($"{name}={value}");
}

return new()
{
IsRootProject = false,
ProjectPath = projectLaunchInfo.ProjectPath,
WorkingDirectory = _projectLauncher.EnvironmentOptions.WorkingDirectory, // TODO: Should DCP protocol specify?
BuildArguments = _buildArguments, // TODO: Should DCP protocol specify?
WorkingDirectory = _projectLauncher.EnvironmentOptions.WorkingDirectory,
BuildArguments = _hostProjectOptions.BuildArguments,
Command = "run",
CommandArguments = arguments,
LaunchEnvironmentVariables = projectLaunchInfo.Environment?.Select(kvp => (kvp.Key, kvp.Value)).ToArray() ?? [],
LaunchEnvironmentVariables = [],
LaunchProfileName = projectLaunchInfo.LaunchProfile,
NoLaunchProfile = projectLaunchInfo.DisableLaunchProfile,
TargetFramework = null, // TODO: Should DCP protocol specify?
TargetFramework = _hostProjectOptions.TargetFramework,
};
}
}
Expand All @@ -250,8 +268,8 @@ private ProjectOptions GetProjectOptions(ProjectLaunchRequest projectLaunchInfo)
public static readonly AspireServiceFactory Instance = new();
public const string AppHostProjectCapability = "Aspire";

public IRuntimeProcessLauncher? TryCreate(ProjectGraphNode projectNode, ProjectLauncher projectLauncher, IReadOnlyList<string> buildArguments)
public IRuntimeProcessLauncher? TryCreate(ProjectGraphNode projectNode, ProjectLauncher projectLauncher, ProjectOptions hostProjectOptions)
=> projectNode.GetCapabilities().Contains(AppHostProjectCapability)
? new SessionManager(projectLauncher, buildArguments)
? new SessionManager(projectLauncher, hostProjectOptions)
: null;
}
2 changes: 1 addition & 1 deletion src/BuiltInTools/dotnet-watch/DotNetWatcher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public override async Task WatchAsync(CancellationToken shutdownCancellationToke
? await browserConnector.LaunchOrRefreshBrowserAsync(projectRootNode, processSpec, environmentBuilder, Context.RootProjectOptions, shutdownCancellationToken)
: null;

environmentBuilder.ConfigureProcess(processSpec);
environmentBuilder.SetProcessEnvironmentVariables(processSpec);

// Reset for next run
buildEvaluator.RequiresRevaluation = false;
Expand Down
51 changes: 13 additions & 38 deletions src/BuiltInTools/dotnet-watch/EnvironmentVariablesBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,6 @@ internal sealed class EnvironmentVariablesBuilder
/// </summary>
private readonly Dictionary<string, string> _variables = [];

/// <summary>
/// Environment variables passed as directives on command line (dotnet [env:name=value] run).
/// Currently, the effect is the same as setting <see cref="_variables"/> due to
/// https://github.com/dotnet/sdk/issues/40484
/// </summary>
private readonly Dictionary<string, string> _directives = [];

public static EnvironmentVariablesBuilder FromCurrentEnvironment()
{
var builder = new EnvironmentVariablesBuilder();
Expand All @@ -42,57 +35,39 @@ public static EnvironmentVariablesBuilder FromCurrentEnvironment()
return builder;
}

public void SetDirective(string name, string value)
{
// should use DotNetStartupHookDirective
Debug.Assert(!name.Equals(EnvironmentVariables.Names.DotnetStartupHooks, StringComparison.OrdinalIgnoreCase));

_directives[name] = value;
}

public void SetVariable(string name, string value)
{
// should use AspNetCoreHostingStartupAssembliesVariable
// should use AspNetCoreHostingStartupAssembliesVariable/DotNetStartupHookDirective
Debug.Assert(!name.Equals(EnvironmentVariables.Names.AspNetCoreHostingStartupAssemblies, StringComparison.OrdinalIgnoreCase));
Debug.Assert(!name.Equals(EnvironmentVariables.Names.DotnetStartupHooks, StringComparison.OrdinalIgnoreCase));

_variables[name] = value;
}

public void ConfigureProcess(ProcessSpec processSpec)
public void SetProcessEnvironmentVariables(ProcessSpec processSpec)
{
processSpec.Arguments = [.. GetCommandLineDirectives(), .. processSpec.Arguments ?? []];
AddToEnvironment(processSpec.EnvironmentVariables);
}

// for testing
internal void AddToEnvironment(Dictionary<string, string> variables)
{
foreach (var (name, value) in _variables)
{
variables.Add(name, value);
}

if (AspNetCoreHostingStartupAssembliesVariable is not [])
foreach (var (name, value) in GetEnvironment())
{
variables.Add(EnvironmentVariables.Names.AspNetCoreHostingStartupAssemblies, string.Join(AssembliesSeparator, AspNetCoreHostingStartupAssembliesVariable));
processSpec.EnvironmentVariables.Add(name, value);
}
}

// for testing
internal IEnumerable<string> GetCommandLineDirectives()
public IEnumerable<(string name, string value)> GetEnvironment()
{
foreach (var (name, value) in _directives)
foreach (var (name, value) in _variables)
{
yield return MakeDirective(name, value);
yield return (name, value);
}

if (DotNetStartupHookDirective is not [])
{
yield return MakeDirective(EnvironmentVariables.Names.DotnetStartupHooks, string.Join(s_startupHooksSeparator, DotNetStartupHookDirective));
yield return (EnvironmentVariables.Names.DotnetStartupHooks, string.Join(s_startupHooksSeparator, DotNetStartupHookDirective));
}

static string MakeDirective(string name, string value)
=> $"[env:{name}={value}]";
if (AspNetCoreHostingStartupAssembliesVariable is not [])
{
yield return (EnvironmentVariables.Names.AspNetCoreHostingStartupAssemblies, string.Join(AssembliesSeparator, AspNetCoreHostingStartupAssembliesVariable));
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,6 @@ public static partial class Names
/// </summary>
public const string DotnetWatchHotReloadNamedPipeName = "DOTNET_WATCH_HOTRELOAD_NAMEDPIPE_NAME";

/// <summary>
/// The full path to the process being launched by dotnet run.
/// Workaround for https://github.com/dotnet/sdk/issues/40484
/// </summary>
public const string DotnetWatchHotReloadTargetProcessPath = "DOTNET_WATCH_HOTRELOAD_TARGET_PROCESS_PATH";

/// <summary>
/// Enables logging from the client delta applier agent.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ namespace Microsoft.DotNet.Watch;
/// </summary>
internal interface IRuntimeProcessLauncherFactory
{
public IRuntimeProcessLauncher? TryCreate(ProjectGraphNode projectNode, ProjectLauncher projectLauncher, IReadOnlyList<string> buildArguments);
public IRuntimeProcessLauncher? TryCreate(ProjectGraphNode projectNode, ProjectLauncher projectLauncher, ProjectOptions hostProjectOptions);
}
48 changes: 25 additions & 23 deletions src/BuiltInTools/dotnet-watch/HotReload/ProjectLauncher.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Globalization;
using Microsoft.Build.Graph;

Expand Down Expand Up @@ -53,17 +54,12 @@ public EnvironmentOptions EnvironmentOptions
{
Executable = EnvironmentOptions.MuxerPath,
WorkingDirectory = projectOptions.WorkingDirectory,
OnOutput = onOutput,
Arguments = [projectOptions.Command, "--no-build", .. projectOptions.CommandArguments]
OnOutput = onOutput
};

var environmentBuilder = EnvironmentVariablesBuilder.FromCurrentEnvironment();
var namedPipeName = Guid.NewGuid().ToString();

// Directives:

// Variables:

foreach (var (name, value) in projectOptions.LaunchEnvironmentVariables)
{
// ignore dotnet-watch reserved variables -- these shouldn't be set by the project
Expand All @@ -85,30 +81,36 @@ public EnvironmentOptions EnvironmentOptions
// expect DOTNET_MODIFIABLE_ASSEMBLIES to be set in the blazor-devserver process, even though we are not performing Hot Reload in this process.
// The value is converted to DOTNET-MODIFIABLE-ASSEMBLIES header, which is in turn converted back to environment variable in Mono browser runtime loader:
// https://github.com/dotnet/runtime/blob/342936c5a88653f0f622e9d6cb727a0e59279b31/src/mono/browser/runtime/loader/config.ts#L330
environmentBuilder.SetDirective(EnvironmentVariables.Names.DotnetModifiableAssemblies, "debug");
environmentBuilder.SetVariable(EnvironmentVariables.Names.DotnetModifiableAssemblies, "debug");

if (injectDeltaApplier)
{
environmentBuilder.DotNetStartupHookDirective.Add(DeltaApplier.StartupHookPath);
environmentBuilder.SetDirective(EnvironmentVariables.Names.DotnetWatchHotReloadNamedPipeName, namedPipeName);

// Do not ask agent to log to stdout until https://github.com/dotnet/sdk/issues/40484 is fixed.
// For now we need to set the env variable explicitly when we need to diagnose issue with the agent.
// Build targets might launch a process and read it's stdout. If the agent is loaded into such process and starts logging
// to stdout it might interfere with the expected output.
//if (context.Options.Verbose)
//{
// environmentBuilder.SetVariable(EnvironmentVariables.Names.HotReloadDeltaClientLogMessages, "1");
//}

// TODO: workaround for https://github.com/dotnet/sdk/issues/40484
var targetPath = projectNode.ProjectInstance.GetPropertyValue("RunCommand");
environmentBuilder.SetVariable(EnvironmentVariables.Names.DotnetWatchHotReloadTargetProcessPath, targetPath);
Reporter.Verbose($"Target process is '{targetPath}'");
environmentBuilder.SetVariable(EnvironmentVariables.Names.DotnetWatchHotReloadNamedPipeName, namedPipeName);

if (context.Options.Verbose)
{
environmentBuilder.SetVariable(EnvironmentVariables.Names.HotReloadDeltaClientLogMessages, "1");
}
}

var browserRefreshServer = await browserConnector.LaunchOrRefreshBrowserAsync(projectNode, processSpec, environmentBuilder, projectOptions, cancellationToken);
environmentBuilder.ConfigureProcess(processSpec);

var arguments = new List<string>()
{
projectOptions.Command,
"--no-build"
};

foreach (var (name, value) in environmentBuilder.GetEnvironment())
{
arguments.Add("-e");
arguments.Add($"{name}={value}");
}

arguments.AddRange(projectOptions.CommandArguments);

processSpec.Arguments = arguments;

var processReporter = new ProjectSpecificReporter(projectNode, Reporter);

Expand Down
2 changes: 1 addition & 1 deletion src/BuiltInTools/dotnet-watch/HotReloadDotNetWatcher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public override async Task WatchAsync(CancellationToken shutdownCancellationToke

var rootProjectNode = evaluationResult.ProjectGraph.GraphRoots.Single();

runtimeProcessLauncher = runtimeProcessLauncherFactory?.TryCreate(rootProjectNode, projectLauncher, rootProjectOptions.BuildArguments);
runtimeProcessLauncher = runtimeProcessLauncherFactory?.TryCreate(rootProjectNode, projectLauncher, rootProjectOptions);
if (runtimeProcessLauncher != null)
{
var launcherEnvironment = runtimeProcessLauncher.GetEnvironmentVariables();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

namespace Microsoft.DotNet.Watch.UnitTests
{
public class EnvironmentVariablesBuilderTest
public class EnvironmentVariablesBuilderTests
{
[Fact]
public void Value()
Expand All @@ -12,10 +12,12 @@ public void Value()
builder.DotNetStartupHookDirective.Add("a");
builder.AspNetCoreHostingStartupAssembliesVariable.Add("b");

var values = new Dictionary<string, string>();
builder.AddToEnvironment(values);
AssertEx.SequenceEqual(["[env:DOTNET_STARTUP_HOOKS=a]"], builder.GetCommandLineDirectives());
AssertEx.SequenceEqual([("ASPNETCORE_HOSTINGSTARTUPASSEMBLIES", "b")], values.Select(e => (e.Key, e.Value)));
var env = builder.GetEnvironment();
AssertEx.SequenceEqual(
[
("DOTNET_STARTUP_HOOKS", "a"),
("ASPNETCORE_HOSTINGSTARTUPASSEMBLIES", "b")
], env);
}

[Fact]
Expand All @@ -27,10 +29,12 @@ public void MultipleValues()
builder.AspNetCoreHostingStartupAssembliesVariable.Add("b1");
builder.AspNetCoreHostingStartupAssembliesVariable.Add("b2");

var values = new Dictionary<string, string>();
builder.AddToEnvironment(values);
AssertEx.SequenceEqual([$"[env:DOTNET_STARTUP_HOOKS=a1{Path.PathSeparator}a2]"], builder.GetCommandLineDirectives());
AssertEx.SequenceEqual([("ASPNETCORE_HOSTINGSTARTUPASSEMBLIES", "b1;b2")], values.Select(e => (e.Key, e.Value)));
var env = builder.GetEnvironment();
AssertEx.SequenceEqual(
[
("DOTNET_STARTUP_HOOKS", $"a1{Path.PathSeparator}a2"),
("ASPNETCORE_HOSTINGSTARTUPASSEMBLIES", "b1;b2")
], env);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ internal class TestRuntimeProcessLauncher(ProjectLauncher projectLauncher) : IRu
{
public class Factory(Action<TestRuntimeProcessLauncher>? initialize = null) : IRuntimeProcessLauncherFactory
{
public IRuntimeProcessLauncher TryCreate(ProjectGraphNode projectNode, ProjectLauncher projectLauncher, IReadOnlyList<string> buildArguments)
public IRuntimeProcessLauncher TryCreate(ProjectGraphNode projectNode, ProjectLauncher projectLauncher, ProjectOptions hostProjectOptions)
{
var service = new TestRuntimeProcessLauncher(projectLauncher);
initialize?.Invoke(service);
Expand Down

0 comments on commit 83c6d4e

Please sign in to comment.