Skip to content

Commit 0833383

Browse files
Copiloteerhardt
Copilot
and
eerhardt
authored
Change install=false semantics to create installer with WithExplicitStart (#13272)
* Initial plan * Implement install=false to use WithExplicitStart for JavaScript and Python installers Co-authored-by: eerhardt <[email protected]> * Add clarifying comments about wait annotation handling Co-authored-by: eerhardt <[email protected]> * Rename variable for better clarity Co-authored-by: eerhardt <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: eerhardt <[email protected]>
1 parent 3d5a06e commit 0833383

File tree

5 files changed

+158
-62
lines changed

5 files changed

+158
-62
lines changed

‎src/Aspire.Hosting.JavaScript/JavaScriptHostingExtensions.cs‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -645,30 +645,20 @@ private static void AddInstaller<TResource>(IResourceBuilder<TResource> resource
645645
var installerName = $"{resource.Resource.Name}-installer";
646646
resource.ApplicationBuilder.TryCreateResourceBuilder<JavaScriptInstallerResource>(installerName, out var existingResource);
647647

648-
if (!install)
648+
if (existingResource is not null)
649649
{
650-
if (existingResource != null)
650+
// Installer already exists, update its configuration based on install parameter
651+
if (!install)
651652
{
652-
// Remove existing installer resource if install is false
653-
resource.ApplicationBuilder.Resources.Remove(existingResource.Resource);
653+
// Remove wait annotation if install is false
654654
resource.Resource.Annotations.OfType<WaitAnnotation>()
655655
.Where(w => w.Resource == existingResource.Resource)
656656
.ToList()
657657
.ForEach(w => resource.Resource.Annotations.Remove(w));
658-
resource.Resource.Annotations.OfType<JavaScriptPackageInstallerAnnotation>()
659-
.ToList()
660-
.ForEach(a => resource.Resource.Annotations.Remove(a));
661-
}
662-
else
663-
{
664-
// No installer needed
665-
}
666-
return;
667-
}
668658

669-
if (existingResource is not null)
670-
{
671-
// Installer already exists
659+
// Add WithExplicitStart to the existing installer
660+
existingResource.WithExplicitStart();
661+
}
672662
return;
673663
}
674664

@@ -695,8 +685,17 @@ private static void AddInstaller<TResource>(IResourceBuilder<TResource> resource
695685
return Task.CompletedTask;
696686
});
697687

698-
// Make the parent resource wait for the installer to complete
699-
resource.WaitForCompletion(installerBuilder);
688+
if (install)
689+
{
690+
// Make the parent resource wait for the installer to complete
691+
resource.WaitForCompletion(installerBuilder);
692+
}
693+
else
694+
{
695+
// Add WithExplicitStart when install is false
696+
// Note: No need to remove wait annotations here since WaitForCompletion was never called
697+
installerBuilder.WithExplicitStart();
698+
}
700699

701700
resource.WithAnnotation(new JavaScriptPackageInstallerAnnotation(installer));
702701
}

‎src/Aspire.Hosting.Python/PythonAppResourceBuilderExtensions.cs‎

Lines changed: 37 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1325,22 +1325,17 @@ private static void AddInstaller<T>(IResourceBuilder<T> builder, bool install) w
13251325
var installerName = $"{builder.Resource.Name}-installer";
13261326
builder.ApplicationBuilder.TryCreateResourceBuilder<PythonInstallerResource>(installerName, out var existingResource);
13271327

1328-
if (!install)
1328+
if (existingResource is not null)
13291329
{
1330-
if (existingResource != null)
1330+
// Installer already exists, update its configuration based on install parameter
1331+
if (!install)
13311332
{
1332-
// Remove existing installer resource if install is false
1333-
builder.ApplicationBuilder.Resources.Remove(existingResource.Resource);
1334-
builder.Resource.Annotations.OfType<PythonPackageInstallerAnnotation>()
1335-
.ToList()
1336-
.ForEach(a => builder.Resource.Annotations.Remove(a));
1333+
// Add WithExplicitStart to the existing installer when install is false
1334+
existingResource.WithExplicitStart();
13371335
}
1338-
return;
1339-
}
1340-
1341-
if (existingResource is not null)
1342-
{
1343-
// Installer already exists, it will be reconfigured via BeforeStartEvent
1336+
// Note: Wait relationships are managed dynamically by SetupDependencies during BeforeStartEvent.
1337+
// No need to remove wait annotations here - SetupDependencies checks for ExplicitStartupAnnotation
1338+
// and skips creating wait relationships when install=false.
13441339
return;
13451340
}
13461341

@@ -1349,6 +1344,13 @@ private static void AddInstaller<T>(IResourceBuilder<T> builder, bool install) w
13491344
.WithParentRelationship(builder.Resource)
13501345
.ExcludeFromManifest();
13511346

1347+
if (!install)
1348+
{
1349+
// Add WithExplicitStart when install is false
1350+
// Note: Wait relationships are managed by SetupDependencies, which checks for ExplicitStartupAnnotation
1351+
installerBuilder.WithExplicitStart();
1352+
}
1353+
13521354
// Add validation for the installer command (uv or python)
13531355
installerBuilder.OnBeforeResourceStarted(static async (installerResource, e, ct) =>
13541356
{
@@ -1479,29 +1481,41 @@ private static void SetupDependencies(IDistributedApplicationBuilder builder, Py
14791481
return; // Resource doesn't exist, nothing to set up
14801482
}
14811483

1484+
// Check if installer has explicit start annotation (install=false was used)
1485+
var shouldSkipInstallerWait = installerBuilder?.Resource.TryGetLastAnnotation<ExplicitStartupAnnotation>(out _) ?? false;
1486+
14821487
// Set up wait dependencies based on what exists:
1483-
// 1. If both venv creator and installer exist: installer waits for venv creator, app waits for installer
1484-
// 2. If only installer exists: app waits for installer
1485-
// 3. If only venv creator exists: app waits for venv creator (no installer needed)
1486-
// 4. If neither exists: app runs directly (no waits needed)
1488+
// 1. If both venv creator and installer exist (and installer doesn't have explicit start): installer waits for venv creator, app waits for installer
1489+
// 2. If both exist but installer has explicit start: app waits for venv creator only
1490+
// 3. If only installer exists (without explicit start): app waits for installer
1491+
// 4. If only venv creator exists: app waits for venv creator (no installer needed)
1492+
// 5. If neither exists or installer has explicit start: app runs directly (no waits needed)
14871493

14881494
if (venvCreatorBuilder != null && installerBuilder != null)
14891495
{
1490-
// Both exist: installer waits for venv, app waits for installer
1491-
installerBuilder.WaitForCompletion(venvCreatorBuilder);
1492-
appBuilder.WaitForCompletion(installerBuilder);
1496+
if (!shouldSkipInstallerWait)
1497+
{
1498+
// Both exist and installer should run automatically: installer waits for venv, app waits for installer
1499+
installerBuilder.WaitForCompletion(venvCreatorBuilder);
1500+
appBuilder.WaitForCompletion(installerBuilder);
1501+
}
1502+
else
1503+
{
1504+
// Installer has explicit start, so only app waits for venv creator
1505+
appBuilder.WaitForCompletion(venvCreatorBuilder);
1506+
}
14931507
}
1494-
else if (installerBuilder != null)
1508+
else if (installerBuilder != null && !shouldSkipInstallerWait)
14951509
{
1496-
// Only installer exists: app waits for installer
1510+
// Only installer exists (without explicit start): app waits for installer
14971511
appBuilder.WaitForCompletion(installerBuilder);
14981512
}
14991513
else if (venvCreatorBuilder != null)
15001514
{
15011515
// Only venv creator exists: app waits for venv creator
15021516
appBuilder.WaitForCompletion(venvCreatorBuilder);
15031517
}
1504-
// If neither exists, no wait relationships needed
1518+
// If neither exists or installer has explicit start, no wait relationships needed for the app
15051519
}
15061520

15071521
private static bool ShouldCreateVenv<T>(IResourceBuilder<T> builder) where T : PythonAppResource

‎tests/Aspire.Hosting.JavaScript.Tests/PackageInstallationTests.cs‎

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,16 +34,21 @@ public void WithNpm_CanBeConfiguredWithInstall()
3434
var installerResources = appModel.Resources.OfType<JavaScriptInstallerResource>().ToList();
3535

3636
Assert.Equal(2, nodeResources.Count);
37-
Assert.Single(installerResources);
37+
// Both should have installer resources now
38+
Assert.Equal(2, installerResources.Count);
3839
Assert.All(nodeResources, resource => Assert.Equal("npm", resource.Command));
3940

4041
// Verify the installer exists for nodeApp
4142
var nodeAppInstallResource = installerResources.Single(r => r.Name == "nodeApp-installer");
4243
Assert.NotNull(nodeAppInstallResource);
43-
44-
// Verify no installer for nodeApp2
45-
var nodeApp2InstallResource = installerResources.SingleOrDefault(r => r.Name == "nodeApp2-installer");
46-
Assert.Null(nodeApp2InstallResource);
44+
// Should NOT have explicit start annotation
45+
Assert.False(nodeAppInstallResource.TryGetLastAnnotation<ExplicitStartupAnnotation>(out _));
46+
47+
// Verify installer exists for nodeApp2 but with explicit start
48+
var nodeApp2InstallResource = installerResources.Single(r => r.Name == "nodeApp2-installer");
49+
Assert.NotNull(nodeApp2InstallResource);
50+
// Should have explicit start annotation
51+
Assert.True(nodeApp2InstallResource.TryGetLastAnnotation<ExplicitStartupAnnotation>(out _));
4752
}
4853

4954
[Fact]
@@ -130,9 +135,12 @@ public async Task WithYarn_DoesNotCreateInstallerWhenInstallIsFalse()
130135
Assert.True(nodeResource.TryGetLastAnnotation<JavaScriptRunScriptAnnotation>(out var _));
131136
Assert.True(nodeResource.TryGetLastAnnotation<JavaScriptBuildScriptAnnotation>(out var _));
132137

133-
// Verify NO installer resource was created
134-
var installerResources = appModel.Resources.OfType<JavaScriptInstallerResource>().ToList();
135-
Assert.Empty(installerResources);
138+
// Verify installer resource WAS created (new behavior)
139+
var installerResource = Assert.Single(appModel.Resources.OfType<JavaScriptInstallerResource>());
140+
Assert.Equal("test-app-installer", installerResource.Name);
141+
142+
// Verify it has explicit start annotation
143+
Assert.True(installerResource.TryGetLastAnnotation<ExplicitStartupAnnotation>(out _));
136144
}
137145

138146
[Fact]
@@ -196,9 +204,12 @@ public async Task WithPnpm_DoesNotCreateInstallerWhenInstallIsFalse()
196204
Assert.True(nodeResource.TryGetLastAnnotation<JavaScriptRunScriptAnnotation>(out var _));
197205
Assert.True(nodeResource.TryGetLastAnnotation<JavaScriptBuildScriptAnnotation>(out var _));
198206

199-
// Verify NO installer resource was created
200-
var installerResources = appModel.Resources.OfType<JavaScriptInstallerResource>().ToList();
201-
Assert.Empty(installerResources);
207+
// Verify installer resource WAS created (new behavior)
208+
var installerResource = Assert.Single(appModel.Resources.OfType<JavaScriptInstallerResource>());
209+
Assert.Equal("test-app-installer", installerResource.Name);
210+
211+
// Verify it has explicit start annotation
212+
Assert.True(installerResource.TryGetLastAnnotation<ExplicitStartupAnnotation>(out _));
202213
}
203214

204215
[Fact]
@@ -292,8 +303,11 @@ public void WithNpmInstallWithYarnNoInstall()
292303
Assert.True(nodeResource.TryGetLastAnnotation<JavaScriptInstallCommandAnnotation>(out var installAnnotation));
293304
Assert.Equal(["install"], installAnnotation.Args);
294305

295-
// the installer resource should NOT be created
296-
Assert.Empty(appModel.Resources.OfType<JavaScriptInstallerResource>());
306+
// the installer resource SHOULD be created (new behavior)
307+
var installerResource = Assert.Single(appModel.Resources.OfType<JavaScriptInstallerResource>());
308+
309+
// Verify it has explicit start annotation
310+
Assert.True(installerResource.TryGetLastAnnotation<ExplicitStartupAnnotation>(out _));
297311
}
298312

299313
[Fact]

‎tests/Aspire.Hosting.JavaScript.Tests/ResourceCreationTests.cs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -179,14 +179,14 @@ public void WithNpmInstallFalseDoesNotCreateInstaller()
179179
Assert.True(nodeResource.TryGetLastAnnotation<JavaScriptRunScriptAnnotation>(out var _));
180180
Assert.True(nodeResource.TryGetLastAnnotation<JavaScriptBuildScriptAnnotation>(out var _));
181181

182-
// Verify NO installer resource was created
183-
var installerResources = appModel.Resources.OfType<JavaScriptInstallerResource>().ToList();
184-
Assert.Empty(installerResources);
182+
// Verify installer resource WAS created (new behavior)
183+
var installerResource = Assert.Single(appModel.Resources.OfType<JavaScriptInstallerResource>());
184+
Assert.Equal("test-app-installer", installerResource.Name);
185185

186-
// Verify no wait annotations were added
187-
Assert.False(nodeResource.TryGetAnnotationsOfType<WaitAnnotation>(out _));
186+
// Verify it has explicit start annotation
187+
Assert.True(installerResource.TryGetLastAnnotation<ExplicitStartupAnnotation>(out _));
188188

189-
// Verify no package installer annotation was added
190-
Assert.False(nodeResource.TryGetLastAnnotation<JavaScriptPackageInstallerAnnotation>(out _));
189+
// Verify no wait annotations were added to the app
190+
Assert.False(nodeResource.TryGetAnnotationsOfType<WaitAnnotation>(out _));
191191
}
192192
}

‎tests/Aspire.Hosting.Python.Tests/AddPythonAppTests.cs‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2287,6 +2287,75 @@ public void MethodOrdering_WithUv_ThenWithPip_ReplacesPackageManager_And_Enables
22872287
Assert.Single(appModel.Resources.OfType<PythonInstallerResource>());
22882288
}
22892289

2290+
[Fact]
2291+
public async Task WithPip_InstallFalse_CreatesInstallerWithExplicitStart()
2292+
{
2293+
using var builder = TestDistributedApplicationBuilder.Create().WithTestAndResourceLogging(outputHelper);
2294+
using var tempDir = new TempDirectory();
2295+
2296+
var scriptPath = Path.Combine(tempDir.Path, "main.py");
2297+
File.WriteAllText(scriptPath, "print('Hello')");
2298+
2299+
var requirementsPath = Path.Combine(tempDir.Path, "requirements.txt");
2300+
File.WriteAllText(requirementsPath, "requests");
2301+
2302+
var pythonApp = builder.AddPythonApp("pythonProject", tempDir.Path, "main.py")
2303+
.WithPip(install: false);
2304+
2305+
var app = builder.Build();
2306+
var appModel = app.Services.GetRequiredService<DistributedApplicationModel>();
2307+
2308+
// Manually trigger BeforeStartEvent to wire up dependencies
2309+
await PublishBeforeStartEventAsync(app);
2310+
2311+
// Verify installer resource WAS created
2312+
var installerResource = Assert.Single(appModel.Resources.OfType<PythonInstallerResource>());
2313+
Assert.Equal("pythonProject-installer", installerResource.Name);
2314+
2315+
// Verify it has explicit start annotation
2316+
Assert.True(installerResource.TryGetLastAnnotation<ExplicitStartupAnnotation>(out _));
2317+
2318+
// Verify the app does NOT wait for the installer
2319+
var pythonAppResource = appModel.Resources.OfType<PythonAppResource>().Single();
2320+
var waitAnnotations = pythonAppResource.Annotations.OfType<WaitAnnotation>().ToList();
2321+
2322+
// Should not wait for installer, only for venv creator
2323+
Assert.All(waitAnnotations, wait => Assert.NotEqual(installerResource, wait.Resource));
2324+
}
2325+
2326+
[Fact]
2327+
public async Task WithUv_InstallFalse_CreatesInstallerWithExplicitStart()
2328+
{
2329+
using var builder = TestDistributedApplicationBuilder.Create().WithTestAndResourceLogging(outputHelper);
2330+
using var tempDir = new TempDirectory();
2331+
2332+
var scriptPath = Path.Combine(tempDir.Path, "main.py");
2333+
File.WriteAllText(scriptPath, "print('Hello')");
2334+
2335+
var pythonApp = builder.AddPythonApp("pythonProject", tempDir.Path, "main.py")
2336+
.WithUv(install: false);
2337+
2338+
var app = builder.Build();
2339+
var appModel = app.Services.GetRequiredService<DistributedApplicationModel>();
2340+
2341+
// Manually trigger BeforeStartEvent to wire up dependencies
2342+
await PublishBeforeStartEventAsync(app);
2343+
2344+
// Verify installer resource WAS created
2345+
var installerResource = Assert.Single(appModel.Resources.OfType<PythonInstallerResource>());
2346+
Assert.Equal("pythonProject-installer", installerResource.Name);
2347+
2348+
// Verify it has explicit start annotation
2349+
Assert.True(installerResource.TryGetLastAnnotation<ExplicitStartupAnnotation>(out _));
2350+
2351+
// Verify the app does NOT wait for the installer
2352+
var pythonAppResource = appModel.Resources.OfType<PythonAppResource>().Single();
2353+
var waitAnnotations = pythonAppResource.Annotations.OfType<WaitAnnotation>().ToList();
2354+
2355+
// Should not have any wait annotations since uv doesn't create venv
2356+
Assert.Empty(waitAnnotations);
2357+
}
2358+
22902359
/// <summary>
22912360
/// Helper method to manually trigger BeforeStartEvent for tests.
22922361
/// This is needed because BeforeStartEvent is normally triggered during StartAsync(),

0 commit comments

Comments
 (0)