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

python@3.10: remove include_dirs, library_dirs from distutils.cfg #91043

Closed

Conversation

mkoeppe
Copy link

@mkoeppe mkoeppe commented Dec 11, 2021

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • 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 --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Homebrew's python3 installs distutils.cfg, which injects include and library paths into the build of every user package. The injected directories appear at the beginning of the paths and hence take precedence over paths configured by the packages' build systems. (We encountered this in the SageMath project in https://trac.sagemath.org/ticket/31335; our workaround is to use SETUPTOOLS_USE_DISTUTILS=local, thus disabling homebrew's copy of distutils.)
In this PR, we remove one item from these paths, with the eventual goal of removing distutils.cfg altogether. distutils.cfg is an outdated and problematic mechanism, which is moreover set to be ineffective for packages that use setuptools when setuptools (again) makes SETUPTOOLS_USE_DISTUTILS=local the standard behavior (pypa/setuptools#2896). See also #76621.

@BrewTestBot BrewTestBot added the legacy Relates to a versioned @ formula label Dec 11, 2021
@dtrodrigues dtrodrigues added CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Dec 11, 2021
@chenrui333 chenrui333 removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Dec 12, 2021
@Bo98
Copy link
Member

Bo98 commented Dec 13, 2021

I'd rather the include/library directories were all dealt with at the same time rather than removing a random one. I definitely agree they should go, even if I'm a little cautious at what we're potentially breaking here.

(I'm aware of the future setuptools issue in regards to prefix but I am already working on a solution for that)

The injected directories appear at the beginning of the paths and hence take precedence over paths configured by the packages' build systems.

More of a side note this, but this is pretty much specific to your use of the compiler-level CPATH. Unless something changed recently, CFLAGS and the use of Extension's include_dirs in setup.py don't behave this way.

@mkoeppe
Copy link
Author

mkoeppe commented Dec 13, 2021

I'd rather the include/library directories were all dealt with at the same time rather than removing a random one. I definitely agree they should go, even if I'm a little cautious at what we're potentially breaking here.

OK, I have made this change

@mkoeppe mkoeppe changed the title Formula/python@3.10.rb: Remove openssl paths from distutils.cfg Formula/python@3.10.rb: Remove include_dirs, library_dirs from distutils.cfg Dec 13, 2021
@SMillerDev SMillerDev changed the title Formula/python@3.10.rb: Remove include_dirs, library_dirs from distutils.cfg python@3.10: remove include_dirs, library_dirs from distutils.cfg Dec 13, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Jan 3, 2022
@mkoeppe
Copy link
Author

mkoeppe commented Jan 4, 2022

ping?

@github-actions github-actions bot removed the stale No recent activity label Jan 4, 2022
@iMichka iMichka added the long build Set a long timeout for formula testing label Jan 17, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Feb 8, 2022
@Bo98
Copy link
Member

Bo98 commented Feb 8, 2022

Apologies for the late response. This has been merged as a part of 3.10.2, but I forgot to mention this issue in the commit. We decided it was best to remove distutils.cfg entirely all at the same time rather than selectively remove parts over time. This took a little bit of time to get right since we needed to modify stuff like sysconfig to ensure we retain the existing install prefix behaviour, as well as work with upstream to make some changes to setuptools. It's all merged now however and we're monitoring if anything has broken so let us know if it makes things better or worse for you. I will post an update to #76621 when I'm comfortable that everything is working fine.

I'm not entirely sure yet what we'll backport to Python 3.9 and earlier. We could backport this bit if needed, but there'll still be some form of distutils.cfg that's around for those versions.

@Bo98 Bo98 closed this Feb 8, 2022
@mkoeppe
Copy link
Author

mkoeppe commented Feb 8, 2022

Thanks for the update!

@github-actions github-actions bot added the outdated PR was locked due to age label Mar 11, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. legacy Relates to a versioned @ formula long build Set a long timeout for formula testing outdated PR was locked due to age stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants