-
Notifications
You must be signed in to change notification settings - Fork 789
Python hosting: Support .venv lookup by walking up parent directories #12616
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
Changes from 8 commits
d729045
faf5a7b
2d05cae
9d9c521
7c32cb4
af30659
a78085f
bb55867
c866034
7881363
26930ab
bf13f27
08eb9ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -76,7 +76,12 @@ public static IResourceBuilder<PythonAppResource> AddPythonApp( | |||||||||||||||||||
| /// <remarks> | ||||||||||||||||||||
| /// <para> | ||||||||||||||||||||
| /// This method executes a Python script directly using <c>python script.py</c>. | ||||||||||||||||||||
| /// By default, the virtual environment folder is expected to be named <c>.venv</c> and located in the app directory. | ||||||||||||||||||||
| /// By default, the virtual environment is resolved using the following priority: | ||||||||||||||||||||
| /// <list type="number"> | ||||||||||||||||||||
| /// <item>If <c>.venv</c> exists in the app directory, use it.</item> | ||||||||||||||||||||
| /// <item>If <c>.venv</c> exists in the AppHost directory (and the app is nearby), use it.</item> | ||||||||||||||||||||
| /// <item>Otherwise, default to <c>.venv</c> in the app directory.</item> | ||||||||||||||||||||
| /// </list> | ||||||||||||||||||||
| /// Use <see cref="WithVirtualEnvironment{T}(IResourceBuilder{T}, string)"/> to specify a different virtual environment path. | ||||||||||||||||||||
| /// Use <c>WithArgs</c> to pass arguments to the script. | ||||||||||||||||||||
| /// </para> | ||||||||||||||||||||
|
|
@@ -353,6 +358,14 @@ private static IResourceBuilder<T> AddPythonAppCore<T>( | |||||||||||||||||||
| // python will be replaced with the resolved entrypoint based on the virtualEnvironmentPath | ||||||||||||||||||||
| var resource = createResource(name, "python", Path.GetFullPath(appDirectory, builder.AppHostDirectory)); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // When using the default virtual environment path, intelligently look for existing virtual environments | ||||||||||||||||||||
|
||||||||||||||||||||
| // in multiple locations: app directory first, then AppHost directory as fallback | ||||||||||||||||||||
| var resolvedVenvPath = virtualEnvironmentPath; | ||||||||||||||||||||
| if (virtualEnvironmentPath == DefaultVirtualEnvFolder && !Path.IsPathRooted(virtualEnvironmentPath)) | ||||||||||||||||||||
|
||||||||||||||||||||
| if (virtualEnvironmentPath == DefaultVirtualEnvFolder && !Path.IsPathRooted(virtualEnvironmentPath)) | |
| if (virtualEnvironmentPath == DefaultVirtualEnvFolder) | |
| ``` #Closed |
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.
Old deprecated methods let you specify the vdir in the AddPythonApp call.
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.
But if the string virtualEnvironmentPath == ".venv", it will never be rooted.
Outdated
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'm not sure the logic here aligns with the comments.
If I have:
- builder.AppHostDirectory =
D:\DotNetTest\RelativePath\RelativePath.AppHost - appWorkingDirectory =
D:\DotNetTest\RelativePath\RelativePath.ApiService
Then Console.WriteLine(Path.GetRelativePath(builder.AppHostDirectory, appWorkingDirectory)); produces:
..\RelativePath.ApiService
Outdated
Copilot
AI
Nov 3, 2025
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.
The comment states the check prevents "completely unrelated location like /tmp or C:\Temp" but Path.IsPathRooted(appDirRelativeToAppHost) would never be true for a relative path returned by Path.GetRelativePath. The condition Path.IsPathRooted(appDirRelativeToAppHost) is redundant since GetRelativePath always returns a relative path. The comment should be updated to accurately reflect that this check is for defensive purposes or the redundant condition should be removed.
| // This is determined by checking if the relative path doesn't start with ".." (going up directories) | |
| // and isn't an absolute path (completely unrelated location like /tmp or C:\Temp). | |
| var appDirRelativeToAppHost = Path.GetRelativePath(builder.AppHostDirectory, appWorkingDirectory); | |
| var isAppDirNearAppHost = !appDirRelativeToAppHost.StartsWith("..", StringComparison.Ordinal) && | |
| !Path.IsPathRooted(appDirRelativeToAppHost); | |
| // This is determined by checking if the relative path doesn't start with ".." (going up directories). | |
| // The check is defensive and ensures the Python app is not located outside or above the AppHost directory. | |
| var appDirRelativeToAppHost = Path.GetRelativePath(builder.AppHostDirectory, appWorkingDirectory); | |
| var isAppDirNearAppHost = !appDirRelativeToAppHost.StartsWith("..", StringComparison.Ordinal); |
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.
For my understanding, the logic here hasn't changed at all. It is just refactored into an intermediate variable. Correct?
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.
What does "and the app is nearby" mean?
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.
Sibling folder or child folder