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

python3: upgrade to 3.8 #54912

Closed
wants to merge 5 commits into from
Closed

python3: upgrade to 3.8 #54912

wants to merge 5 commits into from

Conversation

iMichka
Copy link
Member

@iMichka iMichka commented May 18, 2020

  • Have you followed the guidelines for contributing?
  • 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 <formula>)?

@iMichka
Copy link
Member Author

iMichka commented May 18, 2020

#47274 needs to be finished first.

Copy link
Member

@bayandin bayandin 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 aurora-cli, gradio, and protobuf@3.7 require a revision bump as well

Aliases/python3 Outdated Show resolved Hide resolved
Aliases/python@3 Outdated Show resolved Hide resolved
@Bo98
Copy link
Member

Bo98 commented May 19, 2020

aurora-cli can't be built because the primary source is missing and there's a brew bug preventing the mirror from working. I think it was disabled because of that bug. (That said, I'm pretty sure it doesn't support Python 3 anyway and the dependency shouldn't be there.)

gradio indeed should have a revision bump. Deprecation doesn't mean it can't be installed.

@bayandin
Copy link
Member

Ah, I've missed that these formulae are disabled, thanks for explanation @Bo98!

Actually it's still a bit unclear for me if we need to update disabled/deprecated formulae.

@Bo98
Copy link
Member

Bo98 commented May 19, 2020

We should still treat deprecated formulae like undeprecated formulae in terms of updating like this. Deprecated formulae are still available for install and use and as such should not be intentionally broken if we are aware of an issue (the difference being that we don't actively test it as a dependent). For the end user, deprecation just means a warning when installing.

As for disabled formulae, we should probably still change from python to python@3.7 for style reasons, but no one will actually benefit from the update. We should not force an upgrade via a revision bump as no one will be able to actually upgrade without being blocked by it being disabled.

@iMichka iMichka force-pushed the python3.8 branch 4 times, most recently from c5b93a6 to c4c0122 Compare May 30, 2020 15:37
@iMichka
Copy link
Member Author

iMichka commented May 31, 2020

2020-05-30T16:28:38.2144240Z �[31m==>�[0m �[1m�[31mFAILED�[0m�[0m
2020-05-30T16:28:38.2147610Z �[31mError:�[0m 2 problems in 1 formula detected
2020-05-30T16:28:38.2148060Z python@3.7:
2020-05-30T16:28:38.2148470Z   * python@3.7 is versioned but no python formula exists
2020-05-30T16:28:38.2148800Z   * New formulae in homebrew/core should not have a `bottle do` block

2020-05-30T16:38:14.0911640Z python@3.8:
2020-05-30T16:38:14.0912030Z   * Versioned formulae in homebrew/core should use `keg_only :versioned_formula`
2020-05-30T16:38:14.0912380Z   * Formulae should not have a `HEAD` spec
2020-05-30T16:38:14.0912730Z   * python@3.8 is versioned but no python formula exists

@Bo98
Copy link
Member

Bo98 commented May 31, 2020

* python@3.7 is versioned but no python formula exists
* python@3.8 is versioned but no python formula exists

Add Aliases/python.

* New formulae in homebrew/core should not have a bottle do block

Removing the bottle block for python@3.7 is the easiest solution here. We cannot use existing bottles anyway.

* Versioned formulae in homebrew/core should use keg_only :versioned_formula

Requires a whitelist: Homebrew/brew#7670.

* Formulae should not have a HEAD spec

I don't think we should have HEAD support. --HEAD will install some 3.9 dev version rather than 3.8, which will break most dependents. The formula is also named python@3.8.

@iMichka
Copy link
Member Author

iMichka commented Jun 3, 2020

I made the needed changes.

Just a question about the upgrade for our uses:

  • I installed with brew install python3 back then and I have python 3.7 installed
  • I now run brew update
  • brew outdated shows that nothing has changed

At this point python 3.7 is still installed and in my path. At which point do we upgrade the existing python 3.7 installation to python 3.8 for our users; switching the paths from keg only to non keg only and the other way round?

@Bo98
Copy link
Member

Bo98 commented Jun 3, 2020

@MikeMcQuaid might know. He indicated that brew should remember you installed via an alias and upgrade based on that.

@MikeMcQuaid
Copy link
Member

The source: path in the INSTALL_RECEIPT.json is used to calculate whether it was installed from an alias or not. If it was installed from the alias: that's what we check to determine if it should be upgraded or not. brew install python@3.8 and brew install python will behave differently for upgrades in future, as a result, despite them pointing to the same formula file.

@MikeMcQuaid
Copy link
Member

@iMichka
Copy link
Member Author

iMichka commented Jun 4, 2020

I still need to update the caveats, they are wrong.

And I'll test the upgrade path, I am unsure of what is going on.

@iMichka
Copy link
Member Author

iMichka commented Jun 4, 2020

This is what happens if we merge this PR:

  • Someone ran brew install python before this was merged. He has 3.7 in /usr/local
  • He will probably have the 3.8 keg only version too

When running brew upgrade; the 3.8 symlinking to /usr/local will fail as 3.7 is already there and we are bumping the 3.8 revision.

@MikeMcQuaid
Copy link
Member

@iMichka Can you elaborate a bit more with step-by-step instructions of what to run and what the output is? This should be fixable in Homebrew/brew (and I'm game for doing that).

@iMichka
Copy link
Member Author

iMichka commented Jun 5, 2020

Yes, of course. From a clean state:
brew install python
brew install python@3.8

Then pull this PR.

brew upgrade -s python@3.8

You need to build from source as the bottle is not published, but in real life once this PR is merged the bottle will be there and the -s is not needed.

This upgrade step will fail because it tries to install python@3.8 in the path, and will conflict with the already installed Python 3.7 version.

@SMillerDev
Copy link
Member

@MikeMcQuaid did you ever fix this?

@MikeMcQuaid
Copy link
Member

No.

@MikeMcQuaid
Copy link
Member

@iMichka Thanks. I'd suggest using the link_overwrite DSL to allow all of those conflicting files to be overwritten. This does not require a Homebrew/brew change.

@iMichka
Copy link
Member Author

iMichka commented Jul 3, 2020

Nice. Will try it out.

@iMichka
Copy link
Member Author

iMichka commented Jul 5, 2020

Looks like link_overwrite is not working for this case.

It goes into this else statement and returns false, so nothing is done with the conflicting files:
https://github.com/Homebrew/brew/blob/master/Library/Homebrew/formula.rb#L1121

keg.name == python in that case, and I think the issue is that python.rb has been deleted.

@Rylan12
Copy link
Member

Rylan12 commented Jul 5, 2020

It looks like Formulary.factory("python") follows the python alias and points to python@3.8, causing link_overwrite to return false because it doesn't want to overwrite python@3.8's files.

Would a solution be to allow link_overwrite to succeed if the file it's trying to overwrite belongs to the formula currently being installed?

@MikeMcQuaid
Copy link
Member

Would a solution be to allow link_overwrite to succeed if the file it's trying to overwrite belongs to the formula currently being installed?

I would say in this case it shouldn't even need link_overwrite but: yes.

iMichka added a commit to iMichka/brew that referenced this pull request Jul 5, 2020
See Homebrew/homebrew-core#54912 (comment)
Formulary.factory("python") points to python@3.8, which breaks link_overwrite
for that case.

This changes checks if the formula is an alias, so that we can still override
the files during installation.
@iMichka
Copy link
Member Author

iMichka commented Jul 5, 2020

See Homebrew/brew#7915 for a possible fix.

@iMichka
Copy link
Member Author

iMichka commented Jul 6, 2020

Tested with ly brew fix, link_overwrite now works. We just need a new release.

@Rylan12
Copy link
Member

Rylan12 commented Jul 6, 2020

Just for my own clarification: a release is necessary so that all users will get the latest brew changes (including the necessary link_overwrite change). Merging this PR without the release would mean that non-dev users who don't have the latest brew changes will still get the latest python@3.8 changes but without the necessary link_overwrite changes, causing python@3.8 to link incorrectly.

@iMichka
Copy link
Member Author

iMichka commented Jul 6, 2020

Yes exactly. If you have the DEV flag set as homebrew developer, you are on the latest master and live on the bleeding edge. But shipping this PR without a proper released will break all our user's Python's as they stay on fixed release tags.

Formula/python@3.7.rb Outdated Show resolved Hide resolved
@iMichka iMichka marked this pull request as ready for review July 7, 2020 11:44
@iMichka iMichka requested a review from MikeMcQuaid July 7, 2020 11:44
@BrewTestBot
Copy link
Member

:shipit: @MikeMcQuaid has triggered a merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants