gh-110824 Fix test_sysconfig.test_library on framework builds.

[smontanaro]

The LDLIBRARY config variable is substantially different for framework builds. This PR purports to solve that.

• Issue: test_sysconfig test_library fails on macOS framework installs, like the python.org installer #110824

[smontanaro]

I suspect a skip news label is appropriate here. (Are news entries typically written for small test case fixes?)

[ned-deily]

Thanks for the change. But as I was testing it, I realized that, while with the change, the test will no longer fail with most framework builds, like the python.org installer build, it isn't correct for all framework builds as the name of the framework can be changed with a ./configure option:

  --with-framework-name=FRAMEWORK
                          specify the name for the python framework on macOS
                          only valid when --enable-framework is set. see
                          Mac/README.rst (default is 'Python')

and that determines the file name, so it could end with something other than Python.

But that led me to think more about exactly what this test is trying to do and I'm not certain I know. So I'm going to enquire on the issue and I think we should hold off on trying to fix this test.

[smontanaro]

@ned-deily I'm less concerned with the "why" than the "what." ;-) That said, code coverage would be enough reason to have a test in my book, so maybe that's all it is.

I tweaked the test_library test in the simplest way to accommodate --with-framework-name=....

[ronaldoussoren]

This patch looks good to me.

However... I propose to add the following change as well, because test_get_preferred_schemes changes sys._framework (to an invalid value at that) and doesn't reset it to the original value.

diff --git a/Lib/test/test_sysconfig.py b/Lib/test/test_sysconfig.py
index a19c04b1b2..078b7e00dc 100644
--- a/Lib/test/test_sysconfig.py
+++ b/Lib/test/test_sysconfig.py
@@ -43,6 +43,7 @@ def setUp(self):
         self.name = os.name
         self.platform = sys.platform
         self.version = sys.version
+        self._framework = sys._framework
         self.sep = os.sep
         self.join = os.path.join
         self.isabs = os.path.isabs
@@ -66,6 +67,7 @@ def tearDown(self):
         os.name = self.name
         sys.platform = self.platform
         sys.version = self.version
+        sys._framework = self._framework
         os.sep = self.sep
         os.path.join = self.join
         os.path.isabs = self.isabs
@@ -139,7 +141,7 @@ def test_get_preferred_schemes(self):
         # Mac, framework build.
         os.name = 'posix'
         sys.platform = 'darwin'
-        sys._framework = True
+        sys._framework = "MyPython"
         self.assertIsInstance(schemes, dict)
         self.assertEqual(set(schemes), expected_schemes)

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[smontanaro]

@ronaldoussoren I have made the requested changes; please review again.

[ronaldoussoren]

LGTM

[ronaldoussoren]

But that led me to think more about exactly what this test is trying to do and I'm not certain I know. So I'm going to enquire on the issue and I think we should hold off on trying to fix this test.

I'd prefer to just fix this issue, even if one can question the usefulness of this particular test.

This is yet another example of the underspecified interface for `sysconfig.get_config_var`. It is far from clear to me if this variable is even meant to be useful outside of Python's own build proces. @FFY00 already created https://github.com//issues/103482 to expose information in a cleaner way.

[FFY00]

The fix seems correct to me.

I triggered a re-run of the failed jobs, as the failure seems unrelated, to see if the CI passes. If so, I am happy with this being merged.

[ronaldoussoren]

@smontanaro: Please don't merge main into the PR branch at this point. The PR is fine, and blocked on known flakiness in the Windows 32-bit test runner (which is being looked into). I've restarted that runner and we can merge if that fixes the problem.

[smontanaro]

Apologies.

[ronaldoussoren]

@ned-deily, as you added the DO-NOT-MERGE label: is it ok with you to merge this PR?

[ned-deily]

@ned-deily, as you added the DO-NOT-MERGE label: is it ok with you to merge this PR?

I hate to be picky about this but, the more I think about it, the more I think the current value returned for LDLIBRARY is just plain wrong for framework builds. I can't think of any situation where we should be encouraging anyone to use that sort of path to the framework bundle; the proper UNIXy path is to lib/libpython3.x.dylib and if you were using the -framework option to clang et al you wouldn't use that current LDLIBRARY value anyway. Yes, in most cases, the one is likely currently a symlink to the other but there's no guarantee that will always be true. And, by codifying that path in this test, it could give users the wrong impression.

So, to get past the current test failure and pending a resolution of what LDLIBRARY should really return, I'd like to suggest that we just skip this test in the framework case for now using a self.skipTest("...") as is done elsewhere in test_sysconfig.py. How does that sound?

[ronaldoussoren]

@ned-deily, as you added the DO-NOT-MERGE label: is it ok with you to merge this PR?

I hate to be picky about this but, the more I think about it, the more I think the current value returned for LIBRARY is just plain wrong for framework builds. I can't think of any situation where we should be encouraging anyone to use that sort of path to the framework bundle; the proper UNIXy path is to lib/libpython3.x.dylib and if you were using the -framework option to clang et al you wouldn't use that current LIBRARY value anyway. Yes, in most cases, the one is likely currently a symlink to the other but there's no guarantee that will always be true. And, by codifying that path in this test, it could give users the wrong impression.

So, to get past the current test failure and pending a resolution of what LIBRARY should really return, I'd like to suggest that we just skip this test in the framework case for now using a self.skipTest("...") as is done elsewhere in test_sysconfig.py. How does that sound?

As I've mentioned before, and elsewhere, the real problem here is that the interface for sysconfig.get_config_var() is severely underspecified and should be replaced by more targeted APIs that are well defined and document, such as Python versions of the information returned by the python3-config script including documentation on how they can be used.

We can of course tweak sysconfig.get_config_var("LDLIBRARY") to return libpython3.X.dylib for a framework build, but that will never be valid for all use cases (e.g. in our current setup libpython3.X.dylib is a symlink and can be used for linking, but copying the file and likely won't do what you expect).

And to make live even more interesting: get_config_var("LDLIBRARY") is used in the wild, which means that changing what we return is likely to break as much as it fixed (e.g. fixes linking for anyone that assumes a regular unix installer, but break anything that already knows about frameworks).

I'm afraid there's not much we can do other than accepting the status quo, and wait for someone to be annoyed enough with the current situation to write a PEP for the hypothetical more targeted API I hint at earlier. I'm definitely not up to writing that PEP, and more importantly doing the leg work to (a) find out what the API needs to provide and (b) get buy in on the new API from key sysconfig users (assuming we can identify these).

[ned-deily]

I'm not proposing we change sysconfig in this PR. I'm just proposing a temporary workaround to eliminate the failing test case in the main branch which is why @smontanaro created a PR in the first place. There are already other issues to deal with sysconfig and I will carry the discussion on there. We have until 3.13 beta 1 to find a better solution or accept the status quo.

[ronaldoussoren]

skipping is ok to me.

[ned-deily]

OK. Because I've been the hangup here, I've pushed an update to @smontanaro's PR to skip test_library on framework builds for now. @smontanaro, if you're OK with this, we can merge it and move on. Sorry for the multiple iterations and thanks for spearheading fixing this!

[smontanaro]

Works for me. I was looking no deeper than what I could do to get the test to pass. Nothing more than that.