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

Refactor and document Language::Python. #8379

Merged
merged 2 commits into from
Aug 20, 2020

Conversation

reitermarkus
Copy link
Member

@reitermarkus reitermarkus commented Aug 17, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Extracted from #8339 for easier review.

PYTHON_VIRTUALENV_SHA256_MOJAVE =
"1d7e241b431e7afce47e77f8843a276f652699d1fa4f93b9d8ce0076fd7b0b54"
private_constant :PYTHON_VIRTUALENV_SHA256_MOJAVE

Copy link
Member

Choose a reason for hiding this comment

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

  1. Is there a reason why this is not

    if MacOS.version
      PYTHON_VIRTUALENV_URL = ...
    ...
    else
      PYTHON_VIRTUALENV_URL = ...
    ...
  2. Below we use MacOS.version but there is no require "os/mac/version" or require "os/mac". Are we sure we don't need it on a Mac?

  3. On Linux MacOS.version defaults to NULL, so Linux defaults to using PYTHON_VIRTUALENV_URL_MOJAVE -- I think it's better to make it default to using virtualenv-16.7.4.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really sure how this should behave.

Copy link
Member

@Bo98 Bo98 Aug 17, 2020

Choose a reason for hiding this comment

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

PYTHON_VIRTUALENV_URL_MOJAVE purely exists for system Python 2, which I don't know if we even have support for anymore.

It shouldn't be applied to brewed Python 3 at least.

Copy link
Member

Choose a reason for hiding this comment

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

Can try to kill it here.

end
url "https://files.pythonhosted.org/packages/11/74" \
"/2c151a13ef41ab9fb43b3c4ff9e788e0496ed7923b2078d42cab30622bdf/virtualenv-16.7.4.tar.gz"
sha256 "94a6898293d07f84a98add34c4df900f8ec64a570292279f6d91c781d37fd305"
Copy link
Member

Choose a reason for hiding this comment

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

Probably not in this PR, but we need to consider using a newer version of virtualenv.
Versions prior to v20.0.0 belong to the "legacy" branch (the latest one being 16.7.10 -- maybe we could use it here even now) and the latest stable is: v20.0.31

https://virtualenv.pypa.io/en/latest/changelog.html

Copy link
Member

Choose a reason for hiding this comment

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

Using v20 is complicated as it requires a bunch of dependencies.

16.7.10 should be trivial to update to however.

@reitermarkus reitermarkus merged commit dc07990 into Homebrew:master Aug 20, 2020
@reitermarkus reitermarkus deleted the document-python branch August 20, 2020 02:33
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 16, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants