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

dev-cmd/update-python-resources: use user PATH #16774

Closed
wants to merge 1 commit into from

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Feb 29, 2024

  • 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 is mainly to make sure any extra commands are accessible when running brew update-python-resources. Currently brew bump-formula-pr sets the same PATH:

# As this command is simplifying user-run commands then let's just use a
# user path, too.
ENV["PATH"] = PATH.new(ORIGINAL_PATHS).to_s

Particularly useful in combination with #16772 in order to make brew installed formulae accessible.

Can be reviewed independently from other PR.


One example is borgbackup which runs pkg-config to find libcrypto.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Should we preserve the original value of PATH after the command completes?

@@ -37,6 +37,10 @@ def update_python_resources_args
def update_python_resources
args = update_python_resources_args.parse

# Python packages may need access to additional commands while generating pip report.
Copy link
Member

Choose a reason for hiding this comment

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

Which commands, for example?

Copy link
Member Author

@cho-m cho-m Mar 1, 2024

Choose a reason for hiding this comment

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

Off the top of my head, I recall seeing pkg-config and msgfmt (from gettext). It is usually for Python packages still using setup.py that may end up running these commands.

EDIT: Alternatively, we could go with explicit list and do something like Superenv to only add those opt_bin dirs. I went with simpler option as we already are using this PATH in brew bump / brew bump-formula-pr.

Copy link
Member

@Bo98 Bo98 Mar 5, 2024

Choose a reason for hiding this comment

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

I get setup.py is a script, but why are things invoking tools on a basic metadata parse? Is it just bad scripts calling things outside of build steps?

Are the failures obvious?

Having at least a couple documented examples here might be good.

Copy link
Member

Choose a reason for hiding this comment

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

EDIT: Alternatively, we could go with explicit list and do something like Superenv to only add those opt_bin dirs. I went with simpler option as we already are using this PATH in brew bump / brew bump-formula-pr.

Yeh, I think this might be nicer.

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 get setup.py is a script, but why are things invoking tools on a basic metadata parse? Is it just bad scripts calling things outside of build steps?

Pretty much. One of the issues of having metadata and build commands in the same file as it can bleed over.


Are the failures obvious?

Usually. Though via brew the errors are hidden without --verbose.

brew update-python-resources borgbackup
==> Retrieving PyPI dependencies for "borgbackup==1.2.7"...
Error: Unable to determine dependencies for "borgbackup==1.2.7" because of a failure when running
`/opt/homebrew/opt/[email protected]/libexec/bin/python -m pip install -q --disable-pip-version-check --dry-run --ignore-installed --report=/dev/stdout borgbackup==1.2.7`.
Please update the resources manually.

After verbose, the error at end is:

      OSError: pkg-config probably not installed: FileNotFoundError(2, 'No such file or directory')

Having at least a couple documented examples here might be good.

Sure, if I keep going down this route.


EDIT: Alternatively, we could go with explicit list and do something like Superenv to only add those opt_bin dirs. I went with simpler option as we already are using this PATH in brew bump / brew bump-formula-pr.

Yeh, I think this might be nicer.

When I have a chance, I'll explore this option further. Logic will most likely reside in utils/pypi.rb.

This should also help run brew bump which may allow us to autobump these (so any extra commands are done in CI environment rather than on user system).

Copy link
Member

@Bo98 Bo98 Mar 6, 2024

Choose a reason for hiding this comment

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

Cool, thanks for the example! borgbackup looks like it might improve with 1.4.0b1 but I recognise the need for a broader solution here.

Library/Homebrew/dev-cmd/update-python-resources.rb Outdated Show resolved Hide resolved
@cho-m cho-m force-pushed the update-python-resources-PATH branch from 3e4df98 to 1005a07 Compare March 1, 2024 18:19
@ZhongRuoyu
Copy link
Member

Currently brew bump-formula-pr sets the same PATH

Not entirely related to this PR, but if brew bump-formula-pr is invoked by brew bump as a subprocess, the "original PATH" is just the parent brew bump process's sanitised PATH, not the user's.

This was not the case when the tweak was added in brew bump-formula-pr, but the behaviour was changed in #13593.

@cho-m
Copy link
Member Author

cho-m commented Mar 21, 2024

Closing this. I will work on a different approach that is more restrictive with opt-in flag to reduce unwanted/unexpected commands run on user systems.

@cho-m cho-m closed this Mar 21, 2024
@cho-m cho-m deleted the update-python-resources-PATH branch March 21, 2024 16:34
@github-actions github-actions bot added the outdated PR was locked due to age label Apr 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2024
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.

None yet

5 participants