Skip to content

Changed workon's env switching to use OR not $?#59

Merged
mergify[bot] merged 1 commit intopython-virtualenvwrapper:mainfrom
Nealium:main
May 3, 2023
Merged

Changed workon's env switching to use OR not $?#59
mergify[bot] merged 1 commit intopython-virtualenvwrapper:mainfrom
Nealium:main

Conversation

@Nealium
Copy link
Contributor

@Nealium Nealium commented Apr 13, 2023

Calling type deactivate by itself and using it's exit status breaks if caller is watching for any non-zero exit status.

In my case, a GitLab CI pipeline with a runner calling mkvirtualenv as a Shell command.

Example pipeline output:

Executing "step_script" stage of the job script 00:01
$ . ~/.bashrc
$ mkvirtualenv -r requirements.txt "${ENV_ROOT}_${CI_COMMIT_BRANCH}"
created virtual environment CPython3.10.5.final.0-64 in 338ms
  creator CPython3Posix(dest=/home/gitlab-runner/.virtualenvs/project_a_master, clear=False, no_vcs_ignore=False, global=False)
  seeder FromAppData(download=False, pip=bundle, setuptools=bundle, wheel=bundle, via=copy, app_data_dir=/home/gitlab-runner/.local/share/virtualenv)
    added seed packages: pip==23.0.1, setuptools==67.6.1, wheel==0.40.0
  activators BashActivator,CShellActivator,FishActivator,NushellActivator,PowerShellActivator,PythonActivator
ERROR: Job failed: exit status 1

Tested change on:

  • Rocky 9, bash (Gitlab-Runner)
  • Pop! OS, bash & zsh

dhellmann
dhellmann reviewed Apr 13, 2023
View reviewed changes
virtualenvwrapper.sh
type deactivate >/dev/null 2>&1
if [ $? -eq 0 ]
then
! type deactivate >/dev/null 2>&1 || {
Copy link
Contributor

@dhellmann dhellmann Apr 13, 2023

Choose a reason for hiding this comment

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

Does this { start a sub shell?

Copy link
Contributor Author

@Nealium Nealium May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the delay, sidetracked, was this determined to be in a subshell?

I'm finding the whole topic of determining if it is/isn't confusing and unclear.
$BASH_SUBSHELL seems to indicate it is not. It's not in parentheses and it's not a pipe, so I don't see any reason why it would be

Do subshells potentially raise a bunch of potential problems?- I'm just curious, tidbits and all.

edit: grammar

Copy link
Contributor

@dhellmann dhellmann May 2, 2023

Choose a reason for hiding this comment

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

If it's a sub shell, then actions in the braces don't affect the current shell. Running and then unsetting deactivate won't actually do anything.

https://www.gnu.org/software/bash/manual/html_node/Command-Grouping.html#Command-Grouping seems to indicate that {} does not create a new subshell.

Copy link
Contributor Author

@Nealium Nealium May 3, 2023

Choose a reason for hiding this comment

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

Oh okay! I thought it may have been a compatibility issue, some niche situation or even just bad practice.

I've been daily driving this change, so I know it at least fundamentally works (at least in my env)

dhellmann
dhellmann approved these changes May 3, 2023
View reviewed changes
Copy link
Contributor

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

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

It looks like the test suite is happy, too.

Thanks for this!

@mergify mergify bot merged commit 671587f into python-virtualenvwrapper:main May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhellmann dhellmann dhellmann approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Nealium @dhellmann

Comments