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

formulae*: deprecate formulae which depend on Python 2 #94850

Closed

Conversation

danielnachun
Copy link
Member

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@danielnachun danielnachun added the CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. label Feb 10, 2022
@BrewTestBot BrewTestBot added formula deprecated Formula deprecated macos-only Formula depends on macOS labels Feb 10, 2022
@danielnachun
Copy link
Member Author

In support #93940, these 6 formulae depend on Python 2 and their upstream development appears stalled. I have also set maximum_macos to big_sur to prevent these from being built or installed on Monterey, which will no longer have Python 2 when the 12.3 update is released.

@Bo98
Copy link
Member

Bo98 commented Feb 10, 2022

I have also set maximum_macos to big_sur to prevent these from being built or installed on Monterey

Just to check: do all these require Python 2 at runtime?

@@ -14,8 +14,11 @@ class AppscaleTools < Formula
sha256 cellar: :any, high_sierra: "70e89498336894ae025118e51e418528d8d73da9b1e2786559b6bcbe6055f55b"
end

deprecate! date: "2020-04-20", because: "depends on Python 2"
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for this date? Probably better to just make it today's date so we know when we actually deprecated it.

Copy link
Member Author

@danielnachun danielnachun Feb 10, 2022

Choose a reason for hiding this comment

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

The reason I chose was because it was the day after the release of Python 2.7.18. I happened to see this used for libgraphqlparser, but it seems that this was merged even though there was a desire to use the present date instead. So I will just use today's date instead (for me in the US).

@danielnachun
Copy link
Member Author

I have also set maximum_macos to big_sur to prevent these from being built or installed on Monterey

Just to check: do all these require Python 2 at runtime?

Good question. appscale-tools, git-remote-hg and rfcmarkup appear to be pure Python programs, so I assume they do need Python 2 at runtime. blastem seems to have some support scripts written in Python, but the core program is written in C. libming and rethinkdb are written in C/C++ and have Python 2 bindings.

Maybe we should only set maximum_macos: :big_sur for the 3 formulae that require Python 2 at run time?

@Bo98
Copy link
Member

Bo98 commented Feb 10, 2022

Maybe we should only set maximum_macos: :big_sur for the 3 formulae that require Python 2 at run time?

Yes. If it's Python 2 bindings, then they should just be removed. As for support scripts, depends how critical they are.

If it's mandatory to build (i.e. Python 2 in invoked in build scripts) then there's also maximum_macos: [:big_sur, :build] (or something like that).

@nandahkrishna
Copy link
Member

I just contacted the maintainer of blastem, and it seems like only one Python file (img2tiles.py) is actually run during the build while the other Python scripts are used for testing. They've updated img2tiles.py to be Python 3-compatible, and I've asked if it would be possible to make a release soon.

@danielnachun
Copy link
Member Author

I just contacted the maintainer of blastem, and it seems like only one Python file (img2tiles.py) is actually run during the build while the other Python scripts are used for testing. They've updated img2tiles.py to be Python 3-compatible, and I've asked if it would be possible to make a release soon.

Thanks for looking into that. If they are interested in making a new release that is Python 3 compatible, I'll drop blastem from this PR and we can instead update it in a new PR.

@danielnachun
Copy link
Member Author

Maybe we should only set maximum_macos: :big_sur for the 3 formulae that require Python 2 at run time?

Yes. If it's Python 2 bindings, then they should just be removed. As for support scripts, depends how critical they are.

If it's mandatory to build (i.e. Python 2 in invoked in build scripts) then there's also maximum_macos: [:big_sur, :build] (or something like that).

It does appear that Python bindings can be disabled for libming. I'll test this under Linux where it will obviously fail if it still needs Python 2. If it can be build without the Python bindings, I'll drop it from here and open a PR to disable the bindings instead.

I think I was wrong about how rethinkdb depends on Python 2. It is indeed build-only, and it is because it builds an ancient version of v8 that require Python 2 to compile. There is an open issue for replacing v8 with QuickJS (rethinkdb/rethinkdb#6960) but the PR to implement that hasn't seen activity for 7 months rethinkdb/rethinkdb#6978. Perhaps it's worth posting in the upstream issue that it will no longer be possible to build rethinkdb on macOS 12.3.

@danielnachun
Copy link
Member Author

danielnachun commented Feb 18, 2022

Two small updates for this:

  1. There is actually an upstream commit for blastem that converts that img2tiles.py script to use Python 3: https://www.retrodev.com/repos/blastem/raw-rev/dbbf0100f249. Assuming we are able to apply that to the formula, that would allow us to immediately switch blastem to Python 3.

  2. I found this issue in in the rethinkdb GitHub that pretty clearly indicates the project is now unmaintained because the parent company developing it no longer exists: Rethinkdb status rethinkdb/rethinkdb#6981. It sounds like MongoDB and RavenDB can replace the functionality of RethinkDB, though neither have licenses compatible with packaging for Homebrew. I have no hesitation now in deprecating this formula, though because it only has a build dependency on Python 2, we don't need to set maximum_macos for it, and the existing bottles may continue to work on newer macOS versions for some time.

@danielnachun danielnachun force-pushed the python2_deprecation branch from 2c0c21c to 362da33 Compare March 5, 2022 22:09
@danielnachun
Copy link
Member Author

I've updated this to drop libming and blastem from the PR because they are being updated instead of deprecated. I changed the deprecation date to today and because rethinkdb only has a build a dependency on Python 2, I changed it to depends_on maximum_macos: [:big_sur, :build]. The other 3 formulae need Python 2 at runtime and will not be usable at all in Monterey after the 12.3 update.

@danielnachun danielnachun added the ready to merge PR can be merged once CI is green label Mar 5, 2022
@danielnachun
Copy link
Member Author

I'm going to close this in favor of doing these deprecations individually, as I think it can get too complicated when handling more than one formula.

@danielnachun danielnachun deleted the python2_deprecation branch April 4, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. formula deprecated Formula deprecated macos-only Formula depends on macOS ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants