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

Consider adding PYTHONSTARTUP with shell integration to environment variable collection #23930

Closed
Tyriar opened this issue Aug 8, 2024 · 8 comments · Fixed by #24111
Closed
Assignees
Labels
area-terminal feature-request Request for new features or functionality linux macos triage-needed Needs assignment to the proper sub-team verification-needed Verification of issue is requested verified Verification succeeded

Comments

@Tyriar
Copy link
Member

Tyriar commented Aug 8, 2024

Currently opening the REPL will open the default shell with a custom environment:

this.terminal = this.terminalManager.createTerminal({
name: this.options?.title || 'Python',
env: { PYTHONSTARTUP: this.envVarScript },
hideFromUser: this.options?.hideFromUser,
});

And then run & .../python3 to launch it as a sub-shell:

Screenshot 2024-08-08 at 9 05 14 AM

Why not set the environment variable globally such that any python sub-shell will have shell integration? Adding this to my pwsh profile works:

$env:PYTHONSTARTUP = "/Users/tyriar/.vscode-insiders/extensions/ms-python.python-2024.13.2024080701-darwin-arm64/python_files/pythonrc.py"
Screenshot 2024-08-08 at 9 06 05 AM
@Tyriar Tyriar added the feature-request Request for new features or functionality label Aug 8, 2024
@github-actions github-actions bot added the triage-needed Needs assignment to the proper sub-team label Aug 8, 2024
@anthonykim1 anthonykim1 added area-terminal and removed triage-needed Needs assignment to the proper sub-team labels Aug 21, 2024
anthonykim1 added a commit that referenced this issue Sep 19, 2024
…ollection (#24111)

Resolves: #23930
- setting to opt out of PYTHONSTARTUP injection.

---------

Co-authored-by: Courtney Webster <[email protected]>
@Tyriar
Copy link
Member Author

Tyriar commented Sep 19, 2024

@anthonykim1 are you tracking somewhere the next step for this to change setting defaults?

@anthonykim1
Copy link

@Tyriar I'm not since the default is set to true now in: https://github.com/microsoft/vscode-python/pull/24111/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R635
Thought this was safe enough to make it true out of the gate, given how long shell integration was enabled (with shift+enter) for awhile without complains.

Should we rather start at false and then later change default to true? @Tyriar

@Tyriar
Copy link
Member Author

Tyriar commented Sep 19, 2024

@anthonykim1 it was previously scoped to only users that use the Start REPL comment though, right? Adding this to all terminals when the python extension is installed will massively increase the user base of the feature and also adds unsuspecting users to that list where it's critical we don't break or show an error for. I've always felt there's no rush to roll these things out slowly as the alternative can mean recovery releases and disrupting users.

anthonykim1 added a commit that referenced this issue Sep 19, 2024
From discussion in the original issue:
#23930 (comment)
Making default to be false for September stable, perhaps we could turn
it on to true for insiders AFTER once we ship out stable.
/cc @Tyriar
@anthonykim1
Copy link

Item to eventually change the default is now here: #24141

@anthonykim1 anthonykim1 added this to the September 2024 milestone Sep 23, 2024
@anthonykim1
Copy link

anthonykim1 commented Sep 23, 2024

Verification steps: (Mac and Linux ONLY)

  1. Turn on setting for: "python.terminal.shellIntegration.enabled" to true.
  2. Open your command palette to "Python Clear Cache and Reload")
  3. Type python or (any sort of way you execute your Python Terminal REPL)
  4. see that you have shell integration in your REPL. You will see little command success/failure decorations, access to run recent command, etc
  5. Turn off setting for: "python.terminal.shellIntegration.enabled" to true.
  6. Open your command palette to "Python Clear Cache and Reload")
  7. Type python or (any sort of way you execute your Python Terminal REPL)
  8. make sure you dont see shell integration enabled in the new instance of Python REPL in terminal.

@anthonykim1 anthonykim1 added the verification-needed Verification of issue is requested label Sep 23, 2024
@rzhao271 rzhao271 added the verified Verification succeeded label Sep 24, 2024
@rzhao271
Copy link

Thanks for the clear verification steps, but I'm not seeing any of the shell integration features for Step 4.
I'm on the latest Insiders on Windows and use pwsh by default.

@rzhao271 rzhao271 reopened this Sep 24, 2024
@rzhao271 rzhao271 added verification-found Issue verification failed and removed verified Verification succeeded labels Sep 24, 2024
@github-actions github-actions bot added the triage-needed Needs assignment to the proper sub-team label Sep 24, 2024
@anthonykim1
Copy link

anthonykim1 commented Sep 25, 2024

Sorry for confusion here @rzhao271 Actually you won't see shell integration features if you are on Windows pwsh atm.
This would be only applicable to Mac and Linux users ATM. Issue to track to eventually support this on windows: #24135
Now I'm wondering if we should state this in the setting itself somewhere.

@rzhao271 rzhao271 removed the verification-found Issue verification failed label Sep 25, 2024
@rzhao271 rzhao271 added the verified Verification succeeded label Sep 25, 2024
@rzhao271
Copy link

Verified in WSL with Ubuntu 22.04.

@github-actions github-actions bot removed the info-needed Issue requires more information from poster label Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-terminal feature-request Request for new features or functionality linux macos triage-needed Needs assignment to the proper sub-team verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
3 participants