Skip to content
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

dont automatically inject PYTHONSTARTUP #24346

Merged
merged 46 commits into from
Nov 4, 2024

Conversation

anthonykim1
Copy link

@anthonykim1 anthonykim1 commented Oct 28, 2024

Resolves #24345 and #24290 and #24105
remove automatic injection from before so only thing that allows shell integration to user for Python terminal REPL is the setting itself.

@anthonykim1 anthonykim1 added bug Issue identified by VS Code Team member as probable bug area-repl labels Oct 28, 2024
@anthonykim1 anthonykim1 self-assigned this Oct 28, 2024
@anthonykim1 anthonykim1 changed the title dont automatically inject pystartup dont automatically inject PYTHONSTARTUP Oct 28, 2024
@anthonykim1
Copy link
Author

smoke test that used to only fail for windows is now also failing for linux #24105

@anthonykim1
Copy link
Author

fixed root cause, now further making sure sendText when shell integration is enabled but we never get event from onDidEndShellIntegration.
Added more test

traceVerbose(`Shell Integration is enabled, executeCommand: ${commandLine}`);
}),
// Once shell integration is active, hearing back from onDidEndTerminalShellExecution should not take too long.
new Promise<undefined>((resolve) => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found that without this, smoke test would time out.
Havent heard issue from user about execution taking forever, so in real-life I think onDidEndTerminalShellExecution gets back in reasonable amout of time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still added the promise just in case there is a case where onDidEndTerminalShellExecution never gets fired, we don't want people stuck and waiting for their execution result forever.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would have already scheduled something to run on the terminal. Doesn't sendText mean that it will now add yet another command? Also, in practice Promise.race returns when one of the two finishes, wouldn't it still sendText. there is nothing stopping the setTimeout function from running,

Are you sure that CI is using the new powershell, and not the old one. Have we ensured to set the default shell type to be Powershell when testing this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the smoke tests to use pwsh now :) 12f7b3e
Screenshot 2024-10-30 at 6 53 33 PM

But Im not sure if it is using the newer pwsh or the slower one

@anthonykim1 anthonykim1 marked this pull request as ready for review October 31, 2024 17:42
@vs-code-engineering vs-code-engineering bot added this to the November 2024 milestone Oct 31, 2024
@anthonykim1 anthonykim1 merged commit 3e7e0d2 into microsoft:main Nov 4, 2024
46 checks passed
anthonykim1 added a commit to anthonykim1/vscode-python that referenced this pull request Nov 4, 2024
Resolves microsoft#24345 and
microsoft#24290 and
microsoft#24105
remove automatic injection from before so only thing that allows shell
integration to user for Python terminal REPL is the setting itself.
eleanorjboyd pushed a commit that referenced this pull request Nov 6, 2024
Resolves #24345 and
#24290 and
#24105 remove automatic
injection from before so only thing that allows shell integration to
user for Python terminal REPL is the setting itself.
karthiknadig added a commit that referenced this pull request Nov 12, 2024
…#6)

Resolves #24345 and
#24290 and
#24105 remove automatic
injection from before so only thing that allows shell integration to
user for Python terminal REPL is the setting itself.

Co-authored-by: Anthony Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-repl bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dont automatically inject PYTHONSTARTUP script
2 participants