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

Fix & always include bin/activate in python & poetry plugin #2036

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

carlcsaposs-canonical
Copy link
Contributor

@carlcsaposs-canonical carlcsaposs-canonical commented Dec 12, 2024

Including bin/activate in the virtual environment will make debugging much easier (charm developers can easily run Python code using the charm's virtual environment to debug issues)

@carlcsaposs-canonical
Copy link
Contributor Author

@lengau is this a change you want to include in charmcraft? so that the activate file works by default

or would it be better to include these commands manually in override-build, in case the format of the activate file changes in future python versions?
e.g.

    poetry-keep-bins: true
    override-build: |
      craftctl default
      # Remove venv/bin/ (included because `poetry-keep-bins: true`) except for venv/bin/activate
      shopt -s extglob
      rm -r "$CRAFT_PART_INSTALL/venv/bin/"!(activate)
      # Replace hard-coded path in `venv/bin/activate` with portable path
      # "\&" is escape for sed
      sed -i 's#^VIRTUAL_ENV=.*$#VIRTUAL_ENV="$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )/.." \&> /dev/null \&\& pwd )"#' "$CRAFT_PART_INSTALL/venv/bin/activate"

delete_bins = [
# Remove all files in venv_bin except `activate`
"shopt -s extglob",
f"rm -rf {venv_bin}/!(activate)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lengau
Copy link
Collaborator

lengau commented Dec 12, 2024

@carlcsaposs-canonical with the option to keep all the bins I think we can leave the activate script there just always (if someone really doesn't want it they can unstage it in the charm part). Thanks for the bash magic!

Could you fix the lint and the tests (make setup && make test-fast will get you the prerequisites and run the relevant tests)?

carlcsaposs-canonical added a commit to carlcsaposs-canonical/charmcraft that referenced this pull request Dec 12, 2024
@carlcsaposs-canonical carlcsaposs-canonical changed the title Fix bin/activate and change poetry plugin default to keep bin/activate Fix & always include bin/activate in python & poetry plugin Dec 12, 2024
@carlcsaposs-canonical
Copy link
Contributor Author

@lengau if keep bins is True, should we still modify bin/activate or leave it untouched?

@lengau
Copy link
Collaborator

lengau commented Dec 12, 2024

@carlcsaposs-canonical let's just always modify it.

Including `bin/activate` in the virtual environment will make debugging much easier (charm developers can easily run Python code using the charm's virtual environment to debug issues)
Copy link
Collaborator

@lengau lengau left a comment

Choose a reason for hiding this comment

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

Thanks!

@lengau lengau requested a review from bepri December 16, 2024 23:19
@lengau lengau enabled auto-merge December 16, 2024 23:19
@lengau lengau added this pull request to the merge queue Dec 17, 2024
Merged via the queue into canonical:main with commit 7fa2064 Dec 17, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants