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

poetry: migrate to [email protected] #86776

Closed
wants to merge 2 commits into from

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Oct 7, 2021

poetry: migrate to [email protected]

@BrewTestBot BrewTestBot added the python Python use is a significant feature of the PR or issue label Oct 7, 2021
@henryiii
Copy link
Contributor Author

henryiii commented Oct 7, 2021

I think this is expecting python3 to be valid? Looks like it running on Python 3.5 from the error...

==> /usr/local/Cellar/poetry/1.1.11_1/bin/poetry --version
env: python3: No such file or directory

@chenrui333 chenrui333 added python-3.10-migration CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. linux Linux is specifically affected test failure CI fails while running the test-do block labels Oct 7, 2021
@henryiii
Copy link
Contributor Author

henryiii commented Oct 7, 2021

Because this is not hardcoded in the shabang, it can't use python 3.10 until python3 is part of python 3.10. See #62910.

@SMillerDev
Copy link
Member

We can use rewrite_shebang detected_python_shebang, "file"

@henryiii
Copy link
Contributor Author

henryiii commented Oct 8, 2021

I thought the point of removing the exact homebrew python version was so that people could use homebrew poetry with whatever Python version they had set as python3, not tying it to homebrew python. (Didn't read the issue that carefully, and seemed a little silly to me to use homebrew poetry with non-homebrew python, but not strongly opinionated here)

(Guessing this would replace python3 with the full path again, though correct me if I'm wrong)

@carlocab
Copy link
Member

carlocab commented Oct 8, 2021

I think the idea was to be able to use Homebrew Poetry with Homebrew Python 3.7 or 3.8. I don't think we want to rewrite the shebang here.

Things we could do:

  1. Wait until [email protected] is ready to be made not-keg-only.
  2. Add Homebrew [email protected] to PATH in the test block.

(1) might take a while. (2) means extra setup for some users, but that seems better than a crippled installation.

@henryiii
Copy link
Contributor Author

henryiii commented Oct 8, 2021

(2) would mean "brew install poetry" on a fresh installation would install Python 3.10 and poetry with python3, which wouldn't trigger Python 3.9, which means no python3, so poetry would be broken. (1) seems better.

@carlocab
Copy link
Member

carlocab commented Oct 8, 2021

It seems to work on Catalina and Mojave Big Sur though. I've thought of a third option:

Wrap the poetry binary in an env script that appends [email protected]'s bin to PATH. (It might be possible to skip the env script with some shebang voodoo; not sure.) That way it will always find a python3, but respect the user's choice of one if they have a preference.

@henryiii
Copy link
Contributor Author

henryiii commented Oct 8, 2021

I expect Catalina and Mojave have python3 already, from the developer tools.

@henryiii
Copy link
Contributor Author

henryiii commented Oct 8, 2021

If poetry can be pointed at different pythons, doesn't that apply to python 3.10 too? so if we leave it pointing at python3, we need to keep requiring the package that provides python3 ([email protected]), but users can use 3.10 just like they can use 3.7 and 3.8?

@carlocab
Copy link
Member

carlocab commented Oct 8, 2021

Sorry, I meant Catalina and Big Sur, not Mojave. I thought that Mojave had python3 too. Maybe it's just too old?

@carlocab
Copy link
Member

carlocab commented Oct 8, 2021

If poetry can be pointed at different pythons, doesn't that apply to python 3.10 too? so if we leave it pointing at python3, we need to keep requiring the package that provides python3 ([email protected]), but users can use 3.10 just like they can use 3.7 and 3.8?

Yes. I'm not sure our mixed-version-dependency audit understands this, though, which is an issue for formulae that use poetry but want to depend on [email protected]. Not too many of those, though, so I guess it's not too bad to wait.

Also, simplify the formula's style a little.
@carlocab carlocab added the CI-build-dependents-from-source Pass --build-dependents-from-source to brew test-bot. label Oct 11, 2021
@carlocab
Copy link
Member

Pushed a fix. Added a label to build dependents from source to make sure this didn't break anything.

@carlocab carlocab added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Oct 11, 2021
@henryiii
Copy link
Contributor Author

Looks like python 3.5 is trying to run it on Ubuntu?

@carlocab
Copy link
Member

Ah, yes. That's expected, since Python 3.5 will come before Python 3.10 in PATH. It seems we can do one (or several) of the following:

  1. hardcode a Python version. This will cause regressions for users who now use poetry with a different Python version.
  2. wait until [email protected] is not keg-only.
  3. prepend [email protected]'s bin directory to PATH to fix the test. Users who have an old Python3 early in their PATH would have problems with this formula, but they already would have had these before the changes here. [*]
  4. add some magic to the libexec/"bin/poetry" script that re-executes itself with Python3.10 if it finds it's being used with a Python that's too old.

2 and 4 probably cause the least pain for users, 2 is probably easiest for us, depending on how keen we are to get this running on Python3.10.

[*] This change would make this problem worse, because users who currently have an old Python3 after HOMEBREW_PREFIX/"bin" are fine, but won't be with this change.

@henryiii
Copy link
Contributor Author

Given the feature of being unpinned from a Python version that is causing the problem allows a user to use Python 3.10 already, I'd think option 2 isn't that bad. I'm not against option 1, personally, either, as I see homebrew as only supporting the latest version, and older versions are only there as a workaround due to some packages not supporting newer versions yet. But it's okay with me if people need something different.

br3ndonland added a commit to br3ndonland/dotfiles that referenced this pull request Oct 19, 2021
1d8eee0
884d475

Poetry has a new install script, install-poetry.py, which alters the
requirements for adding Poetry to `$PATH`. `$HOME/.local/bin` was
already on `$PATH` for pipx, so it seemed like a good option. Commits
1d8eee0 and 884d475 updated `.zshrc` and `script/strap-after-setup` for
install-poetry.py and `POETRY_HOME=$HOME/.local`.

This made sense initially, because Poetry installs its binaries into
`$POETRY_HOME/bin`, and because Poetry doesn't have a `$POETRY_BIN_DIR`
configuration variable like pipx does (`$PIPX_BIN_DIR`). Unfortunately,
`POETRY_HOME=$HOME/.local` ended up being problematic, because Poetry
takes over `$POETRY_HOME`, and doesn't consider other applications
installed there. For example, if the get-poetry.py or install-poetry.py
scripts were used to install Poetry, they can also be used to uninstall
Poetry. Uninstalling with `python install-poetry.py --uninstall` or
`python get-poetry.py --uninstall` deletes the entire `$POETRY_HOME`
directory, which means it deletes `$HOME/.local`, causing problems for
other applications that use `$HOME/.local` (python-poetry/poetry#4625).

There have been many other issues with the Poetry custom install scripts
get-poetry.py and install-poetry.py (br3ndonland/inboard#36), so other
installation methods are be welcome.

Poetry is now available through Homebrew, but Homebrew installation is
not supported by the Poetry maintainers. Homebrew installation also
requires its own custom install script, which creates its own issues.
python-poetry/poetry#941
python-poetry/poetry#1765
Homebrew/homebrew-core#48883
Homebrew/homebrew-core#86776

pipx (https://pypa.github.io/pipx/) can also be used to install Poetry.
The pipx installation method is suggested in the Poetry docs and GitHub,
and pipx is already in use in this repo.
python-poetry/poetry#677
python-poetry/poetry#3360

This commit will remove `export POETRY_HOME=$HOME/.local` from `.zshrc`,
and will install Poetry with pipx.
@branchvincent
Copy link
Member

4 would look something like:

diff --git a/Formula/poetry.rb b/Formula/poetry.rb
index ae859e2d30f..81507a945d0 100644
--- a/Formula/poetry.rb
+++ b/Formula/poetry.rb
@@ -204,6 +204,7 @@ class Poetry < Formula
 
     # We don't hardcode Homebrew Python here on purpose. See
     # https://github.com/Homebrew/homebrew-core/issues/62910
+    python310 = Formula["[email protected]"].opt_bin/"python3"
     (libexec/"bin/poetry").atomic_write <<~PYTHON
       #!/usr/bin/env python3
       import sys
@@ -211,6 +212,10 @@ class Poetry < Formula
       sys.path.insert(0, "#{site_packages}")
       sys.path.insert(0, "#{vendor_site_packages}")
 
+      if sys.version_info[:2] < (3, 6):
+          import os
+          os.execv("#{python310}", ("#{python310}",) + tuple(sys.argv))
+
       if __name__ == "__main__":
           from poetry.console import main
           main()

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Nov 15, 2021
@github-actions github-actions bot closed this Nov 22, 2021
@github-actions github-actions bot added the outdated PR was locked due to age label Dec 22, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-build-dependents-from-source Pass --build-dependents-from-source to brew test-bot. CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. linux Linux is specifically affected outdated PR was locked due to age python Python use is a significant feature of the PR or issue python-3.10-migration stale No recent activity test failure CI fails while running the test-do block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants