Python: Change Homebrew includes into NODIST flags

[mitchhentges]

• Have you followed the guidelines for contributing?
• 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 <formula>)?

---

Python CFLAGS are put before all other arguments when compiling extensions for pip packages. This is causing collisions between vendored code in the pip package and libraries installed via brew.
This patch:

• Moves the brew include path to [C|LD]FLAGS_NODIST
• Changes SDK imports from -I to -isystem to reduce their priority
• Removes extra SDK import, since it's implied by -isysroot

Fixes #68352

[carlocab]

The Tcl/Tk tests are skipped on 3.8 and 3.9 for Big Sur. See #67378 for more details.

If you want to include 3.7 in this PR you need to modify 3.7's test in the same way. (3.7 won't get bottled if its tests fail.) You should probably also fix your commit messages, as they look like they were cut off.

[mkoeppe]

Same should be done for the -L flags, I think – replace them by using LIBRARY_PATH.

[carlocab]

Also, #68365 has been merged. You'll need to rebase then give the [email protected] revision another bump again.

[fxcoudert]

@alebcay I would welcome your opinion on this

[alebcay]

This is a bit beyond my knowledge, so I don't really have much to add, but it looks like a fairly trivial patch in terms of what it fixes, which is good. As long as there are no regressions from dependent formulae, I have no objections.

[mitchhentges]

If you want to include 3.7 in this PR you need to modify 3.7's test in the same way.

👍

You should probably also fix your commit messages, as they look like they were cut off.

I put the term $CPATH in my message, and it seems that bash helped me out by expanding the empty variable. Thanks for the catch :)

Same should be done for the -L flags, I think – replace them by using LIBRARY_PATH.

@mkoeppe Can you elaborate here? Are the LDFLAGS also influencing pip package installations? If not, I'd like to keep this PR smaller, if possible.

Also, #68365 has been merged. You'll need to rebase then give the [email protected] revision another bump again.

Radical, on it.

but it looks like a fairly trivial patch in terms of what it fixes, which is good. As long as there are no regressions from dependent formulae, I have no objections.

👍 cheers mate

[mkoeppe]

@mkoeppe Can you elaborate here? Are the LDFLAGS also influencing pip package installations? If not, I'd like to keep this PR smaller, if possible.

Yes, all of the variables listed here:
https://github.com/pypa/setuptools/blob/main/setuptools/_distutils/sysconfig.py#L208

            get_config_vars('CC', 'CXX', 'CFLAGS',
                            'CCSHARED', 'LDSHARED', 'SHLIB_SUFFIX', 'AR', 'ARFLAGS')

are taken from sysconfig. The configure-time value of LDFLAGS goes into LDSHARED, as you can see:

$ /usr/local/bin/python3 -m sysconfig | grep LDSHARED
	BLDSHARED = "clang -bundle -undefined dynamic_lookup -L/usr/local/lib -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk"
	LDSHARED = "clang -bundle -undefined dynamic_lookup -L/usr/local/lib -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk"

Like -I flags, also -L flags are read from left to right, so this is as critically broken and should be fixed in the same way.

[mitchhentges]

Setting up LIBRARY_PATH and running checks locally again here.

[mitchhentges]

Every usage of -I in CFLAGS/CPPFLAGS has been moved to CPATH, every usage of -L in LD_FLAGS has been moved to LIBRARY_PATH

[mkoeppe]

Looks good to me

[carlocab]

This broke something, critically:

==> /opt/homebrew/Cellar/[email protected]/3.9.1_6/bin/python3.9 -c import _gdbm
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named '_gdbm'

==> /opt/homebrew/Cellar/[email protected]/3.8.7_1/bin/python3.8 -c import _gdbm
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named '_gdbm'

[mitchhentges]

Yep, currently investigating. @carlocab two questions:

1. Where can I see the logs for the formula's build logs?
2. I wonder if the tests are working for non-ARM. I haven't every seen the 11/10.15/10.14 tasks finish, do you know if there's an active unrelated issue with them?

[carlocab]

Where can I see the logs for the formula's build logs?

If you wait for all the tests to complete failure logs are uploaded as artifacts on GitHub's UI.

I wonder if the tests are working for non-ARM. I haven't every seen the 11/10.15/10.14 tasks finish, do you know if there's an active unrelated issue with them?

Well, if you consider running on slow Intel hardware an active issue, then, yes. I would be surprised if the tests perform differently on the Intel runners.

[carlocab]

Actually, I take it back. I think the [email protected] and [email protected] tests succeeded on the Intel runners, as a quick peek at CI showed that it was already testing something else.

[fxcoudert]

Python not finding gdbm on ARM is why the CFLAGS/LDFLAGS fix was first introduced, so… not surprising

[mitchhentges]

Hmm, I don't have an ARM machine locally here, so I'm going to dump some commits on this PR so I can test via that CI job.

[carlocab]

If you want to test whether your changes will work locally, you don't need an ARM machine. Just install Homebrew into a non-default prefix, say, $HOME/homebrew, and try to install and test Python using your non-default Homebrew install.

https://docs.brew.sh/Installation#untar-anywhere

Python CI runs take just over 24 hours to complete, so using them to experiment isn't really efficient.

[mkoeppe]

python's setup.py uses hardcoded directories where it's looking for Berkeley db.
https://github.com/python/cpython/blob/master/setup.py#L1140

[mkoeppe]

This will need to be patched out anyway if you don't want random stuff from users' Fink installations (/sw/include/db4 etc.) to be picked up on brew install --build-from-source python3

[fxcoudert]

This will need to be patched out

We really want to patch as few things as possible, it's not a sustainable long-term solution for managing formulas.

setup.py looks for the db libraries in inc_dirs, which is the mechanism planned for finding in non-standard locations.

if you don't want random stuff from users' Fink installations

We can't support every user configuration, we aim for things that build on our clean CI. Homebrew is not primarily a build-from-source package manager, but a binary package manager that happens to support build-from-source in some cases.

[mkoeppe]

This will need to be patched out

setup.py looks for the db libraries in inc_dirs, which is the mechanism planned for finding in non-standard locations.

You are right, the hardcoded db_inc_paths are only used as fallback and are checked after inc_dirs.

[zlscherr]

@mitchhentges For [email protected], how about something like:

diff --git a/Formula/[email protected] b/Formula/[email protected]
index fb33f58fe1..4306ac0f4e 100644
--- a/Formula/[email protected]
+++ b/Formula/[email protected]
@@ -100,9 +100,11 @@ class PythonAT38 < Formula
       --with-openssl=#{Formula["[email protected]"].opt_prefix}
     ]

-    cflags   = ["-I#{HOMEBREW_PREFIX}/include"]
-    ldflags  = ["-L#{HOMEBREW_PREFIX}/lib"]
+    cflags_nodist   = ["-I#{HOMEBREW_PREFIX}/include"]
+    ldflags_nodist  = ["-L#{HOMEBREW_PREFIX}/lib"]
     cppflags = ["-I#{HOMEBREW_PREFIX}/include"]
+    cflags = []
+    ldflags = []

     if MacOS.sdk_path_if_needed
       # Help Python's build system (setuptools/pip) to build things on SDK-based systems
@@ -138,9 +140,11 @@ class PythonAT38 < Formula
       f.gsub! "DEFAULT_FRAMEWORK_FALLBACK = [", "DEFAULT_FRAMEWORK_FALLBACK = [ '#{HOMEBREW_PREFIX}/Frameworks',"
     end

+    args << "CFLAGS_NODIST=#{cflags_nodist.join(" ")}" unless cflags_nodist.empty?
+    args << "LDFLAGS_NODIST=#{ldflags_nodist.join(" ")}" unless ldflags_nodist.empty?
+    args << "CPPFLAGS=#{cppflags.join(" ")}" unless cppflags.empty?
     args << "CFLAGS=#{cflags.join(" ")}" unless cflags.empty?
     args << "LDFLAGS=#{ldflags.join(" ")}" unless ldflags.empty?
-    args << "CPPFLAGS=#{cppflags.join(" ")}" unless cppflags.empty?

     system "./configure", *args
     system "make"

you maybe need to retain the CFLAGS and LDFLAGS for the SDK section, but then move HOMEBREW_PREFIX/include and HOMEBREW_PREFIX/lib to CFLAGS_NODIST and LDFLAGS_NODIST. This lets me build/test [email protected] successfully, although I'm not on an ARM machine. You might want to do something similar for [email protected].

[mitchhentges]

I think CI looks happy, the only python-related failure was [email protected] on 11.0, which is expected because Big Sur compatibility has not (yet) been backported to py3.7

[carlocab]

the only python-related failure was [email protected] on 11.0

This is blocking though, as merging this will break the existing Big Sur bottle for [email protected]. It would be a different story if 3.7 didn't have a Big Sur bottle, but it does...

[mitchhentges]

the only python-related failure was [email protected] on 11.0

This is blocking though, as merging this will break the existing Big Sur bottle for [email protected]. It would be a different story if 3.7 didn't have a Big Sur bottle, but it does...

I wonder how [email protected] has been successfully bottled for Big Sur so far.

[carlocab]

CI failures look exactly the same as #69668, so this LGTM.

Thanks for your first contribution to homebrew/core, @mitchhentges. I hope it's the first of many high-quality contributions to come.

[carlocab]

We forgot the [email protected] revision bump. I'll make sure that's done in #70177.

[mkoeppe]

Thanks very much for this work. This seems to work well.

[carlocab]

Glad it works for you, @mkoeppe.

Thanks again for all the work you put into this, @mitchhentges!

[zlscherr]

For the record, the brew upgrade didn't change the results of

python3-config --cflags.

I needed to manually do brew reinstall [email protected] to see the changes.

[carlocab]

That's because

We forgot the [email protected] revision bump.

That'll be fixed when #70177 is merged.