Skip to content

bump-formula-pr, utils/pypi: tweak log messages#8149

Merged
SeekingMeaning merged 1 commit intoHomebrew:masterfrom
SeekingMeaning:python-logging
Aug 7, 2020
Merged

bump-formula-pr, utils/pypi: tweak log messages#8149
SeekingMeaning merged 1 commit intoHomebrew:masterfrom
SeekingMeaning:python-logging

Conversation

@SeekingMeaning
Copy link
Copy Markdown
Contributor

@SeekingMeaning SeekingMeaning commented Jul 31, 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?

More useful in combination with #8147

  • bump-formula-pr - pass in args.quiet? to update_python_resources! instead of always passing in true for silent
  • utils/pypi
    • Add parentheses that say that retrieving PyPI dependencies "may take awhile"
    • Add message about getting PyPI info for each dependency to give a better sense of what is happening behind the scenes

Comment thread Library/Homebrew/utils/pypi.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
ohai "Retrieving PyPI dependencies for \"#{pypi_name}==#{version}\" (this may take awhile)" if !print_only && !silent
ohai "Retrieving PyPI dependencies for \"#{pypi_name}==#{version}\"..." if !print_only && !silent

@dtrodrigues
Copy link
Copy Markdown
Member

One thing I've noticed is that it always prints brew update-python-resources <formulaname>, even for non-python formulae. It may be worth changing where that message is logged because even though the command technically is called every time, it's a little confusing to always see.

@SeekingMeaning
Copy link
Copy Markdown
Contributor Author

Maybe we should skip PyPI.update_python_resources! if the formula doesn't depend on any version of python

It seems all formulae using pythonhosted require python: https://github.com/Homebrew/homebrew-core/search?q=pythonhosted+NOT+python

@Rylan12
Copy link
Copy Markdown
Member

Rylan12 commented Aug 5, 2020

There are 16 formulae that use files.pythonhosted.org but not depends_on "python:

$ grep -L 'depends_on "python' $(grep -l 'files.pythonhosted.org' *)
appscale-tools.rb
blastem.rb
bup.rb
cassandra.rb
cassandra@2.1.rb
cassandra@2.2.rb
gr-osmosdr.rb
mesos.rb
numpy@1.16.rb
offlineimap.rb
ooniprobe.rb
pypy.rb
pypy3.rb
python@3.7.rb
python@3.8.rb
volatility.rb

Most of the are because they have depends_on :macos for python 2.

There are still a few that don't declare a python dependency though and have resources:

  • gr-osmosdr (although python@3.8 is a recursive dependency)
  • pypy
  • pypy3
  • python@3.7
  • python@3.8

@SeekingMeaning
Copy link
Copy Markdown
Contributor Author

Most of them are because they have depends_on :macos for python 2.

Maybe Python should have something like Java's depends_on java: "1.8", perhaps depends_on python: "2"? Also maybe we should do this for Ruby too as some formula (or at least they used to) depend on built-in macOS Ruby

There are still a few that don't declare a python dependency though and have resources:

  • gr-osmosdr (although python@3.8 is a recursive dependency)
  • pypy
  • pypy3
  • python@3.7
  • python@3.8

We probably could just hardcode pypy and versioned python since they're obviously Python-related packages

@jonchang
Copy link
Copy Markdown
Contributor

jonchang commented Aug 6, 2020

Maybe Python should have something like Java's depends_on java: "1.8", perhaps depends_on python: "2"? Also maybe we should do this for Ruby too as some formula (or at least they used to) depend on built-in macOS Ruby

We're trying to move away from Requirements in Homebrew/core. I think the approach of having a hardcoded list of Python formulae to check against for the purposes of updating Python resources is fine. We do that in languages/python.rb anyway.

@MikeMcQuaid
Copy link
Copy Markdown
Member

One thing I've noticed is that it always prints brew update-python-resources <formulaname>, even for non-python formulae. It may be worth changing where that message is logged because even though the command technically is called every time, it's a little confusing to always see.

How slow is this to run? If it's not slow: let's just not print that it's run unless something is changed in the files. This seems preferable to maintaining a allowlist/denylist just to avoid a message being printed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
ohai "brew update-python-resources #{formula.name}"

This line probably isn't needed anymore if we pass silent: args.quiet? to PyPI.update_python_resources! (whereas previously we always passed silent: true)

Example output without PyPI dependencies:

$ brew bump-formula-pr helmfile --version 0.125.3 --dry-run --write --force
...
==> Downloading https://github.com/roboll/helmfile/archive/v0.125.3.tar.gz
==> replace /https:\/\/github\.com\/roboll\/helmfile\/archive\/v0\.125\.2\.tar\.gz/ with "https://github.com/roboll/helm
==> replace "72d3b5646459408b8096c94408ea5fdf72c335575b99e3c0072fe81320b98c6c" with "2ca6b039c72414eaf2ffe5848f61320f8ae
==> brew audit helmfile.rb
...

With PyPI dependencies:

$ brew bump-formula-pr jc --version 1.13.4 --dry-run --write --force
...
==> Downloading https://files.pythonhosted.org/packages/e3/b9/7878a4f71c873c7d67f39615086f1c8315740534b25eddc1a4f75f3148
==> replace /https:\/\/files\.pythonhosted\.org\/packages\/2b\/1e\/179eea9186313bcff8dc3405ecd4615043ea615681519a9638d79
==> replace "b5ebb419b3b5d3cd95a166b4f156c7986235e983d5c6bf21aa9d57586e211f78" with "45480ac3d399f70b57d8cc97a6795ea875a
==> Retrieving PyPI dependencies for "jc==1.13.4"...
==> Getting PyPI info for "pygments==2.6.1"
==> Getting PyPI info for "ruamel-yaml==0.16.10"
==> Getting PyPI info for "ruamel-yaml-clib==0.2.0"
==> Getting PyPI info for "xmltodict==0.12.0"
==> Updating resource blocks
==> brew audit jc.rb
...

@SeekingMeaning SeekingMeaning changed the title bump-formula-pr, utils/pypi: more log messages bump-formula-pr, utils/pypi: tweak log messages Aug 6, 2020
@SeekingMeaning SeekingMeaning merged commit cab2c99 into Homebrew:master Aug 7, 2020
@SeekingMeaning SeekingMeaning deleted the python-logging branch August 7, 2020 15:18
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 19, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 19, 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.

6 participants