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

utils/pypi: exclude deps of excluded packages #15896

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

branchvincent
Copy link
Member

  • 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 typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This teaches brew update-python-resources to also ignore the dependencies of excluded packages (since we exclude packages provided instead by formulae, which of course already contain these required deps).

In core, our exclude_packages in pypi_formula_mappings.json started out with simple packages without dependencies like six where this distinction didn't matter. But now, our use has grown to exclude packages with their own dependencies (cryptography, keyring, and virtualenv). So this change reduces the manual maintainence of our formula mappings.

Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense to me, thanks @branchvincent!

@MikeMcQuaid MikeMcQuaid merged commit 4564628 into Homebrew:master Aug 22, 2023
24 checks passed
@wellson
Copy link

wellson commented Aug 22, 2023

I felt like it broke the brew. Merge problem

fatal: couldn't find remote ref refs/heads/master
Error: Fetching /opt/homebrew/Library/Taps/dart-lang/homebrew-dart failed!
Installing from the API is now the default behaviour!
You can save space and time by running:
brew untap homebrew/core
brew untap homebrew/cask

Error: Some taps failed to update!

The following taps can not read their remote branches:

dart-lang/dart

This is happening because the remote branch was renamed or deleted.
Reset taps to point to the correct remote branches by running brew tap --repair
Failed during: /opt/homebrew/bin/brew update --force --quiet

# Resolve the dependency tree of excluded packages to prune the above
exclude_packages.delete_if { |package| found_packages.exclude? package }
ohai "Retrieving PyPI dependencies for excluded \"#{exclude_packages.join(" ")}\"..." if !print_only && !silent
exclude_packages = pip_report(exclude_packages) + [Package.new(main_package.name)]
Copy link
Member

Choose a reason for hiding this comment

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

This might incorrectly exclude common deps shared by the main package and some of the exclude_packages, I think.

Take Homebrew/homebrew-core#140275 as an example (see Homebrew/homebrew-core@f4c55ea). Here, moto, the main package, depends on boto3 and botocore. One of its exclude_packages, cfn-lint, also depends on boto3 and botocore. IIUC, with this logic, boto3 and botocore are incorrectly removed.

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 think that's what we want, since in python there's only a single version of a dep shared by the whole venv (so no need to install it twice).

That formula actually has a bug, its depending on cfn-lint without it being importable so neither is boto3 (it needs a .pth file to find it)

$(brew --prefix)/opt/moto/libexec/bin/python -c 'import cfnlint'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'cfnlint'

@drewfish
Copy link

drewfish commented Aug 28, 2023

Hmm.... I've setup a tap for company-internal tools. (I realize that isn't the goal of homebrew so I'm kinda on my own.) I have a formula for a python CLI which depends on another company-internal python library. That company-internal python library is github private repo.

I previously had done brew update-python-resources --print-only private-python-cli --ignore-non-pypi-packages --exclude-packages=private-python-lib and that worked fine. After this PR, it gives me this error

Retrieving PyPI dependencies for excluded "private-python-lib"...Error: Unable to determine dependencies for "private-python-lib" because of a failure when running
/usr/local/opt/[email protected]/bin/python3 -m pip install -q --dry-run --ignore-installed --report=/dev/stdout private-python-lib.

In the formula for private-python-cli I've hand-added this:

  resource "private-python-lib" do
    url "ssh://[email protected]/mycompany/private-python-lib.git",
      using:    :git,
      tag:      "v1.2.1"
  end

I think a fix for this might be to have the "look up dependencies of excluded packages" logic skip non-pypi packages (when --ignore-non-pypi-packages is passed). Does that seem right? If so I can attempt a PR.

[edit]

OK I attempted that fix I suggested. A relatively-simple approach didn't work, I suspect because the mix/confusion of simple package names (e.g. python-dateutil) and packages specified as URLs (e.g. my git+ssh://[email protected]/mycompany/private-python-lib.git).

Given that I'm trying to use homebrew in a way outside of its goals, I think I should figure out my own solution to this (and not try to change update-python-resources to also support private taps/formulae).

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