-
Notifications
You must be signed in to change notification settings - Fork 51
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
Build changes from MacPorts #368
base: master
Are you sure you want to change the base?
Conversation
This prevents passing e.g. -j to the build
Should one of the modules fail, the build would just print an error, but still exit with a success.
This is generally unsuitable for distribution, as it may cause failures on e.g. deprecation.
d539aa4
to
ec8c4c3
Compare
@@ -15,7 +15,7 @@ | |||
import shutil | |||
import subprocess | |||
import sys | |||
from distutils.sysconfig import get_config_var | |||
from setuptools._distutils.sysconfig import get_config_var |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look correct, the new version of the code uses a private submodule of setuptools
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't use this bit, I've changed the script to use sysconfig.get_config_var
.
@@ -122,7 +122,7 @@ def get_sdk_level(): | |||
assert sdkname.startswith("MacOSX") | |||
assert sdkname.endswith(".sdk") | |||
|
|||
if sdkname == "MacOSX.sdk": | |||
if sdkname == "MacOSX.sdk" or "." not in sdkname[6:-4]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because MacOSX11.sdk exists as a valid option at /Library/Developer/CommandLineTools/SDKs
. This particular one affects Homebrew as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on a change that will turn the logic around: use SDKSettings.plist where it is available and only use the version in the base name of the SDK when there is no plist file. Both here and in the various setup files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. We also primarily use SDKSettings in Homebrew's SDK selection and it's worked well so it should work for PyObjC too. Just be aware you may need to strip the patch version like you do to the OS version (there was a 10.15.4 SDK).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stripping shouldn't be necessary, the code should work fine with more than 3 labels....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
db2cdc8 implements the new code.
@@ -6,7 +6,7 @@ | |||
""" | |||
|
|||
import platform | |||
from distutils.version import LooseVersion | |||
from setuptools._distutils.version import LooseVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above, private submodule of setuptools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changeset 12daa01 removes the dependency on distutils in this file.
@@ -15,7 +15,7 @@ | |||
import shutil | |||
import subprocess | |||
import sys | |||
from distutils.sysconfig import get_config_var | |||
from setuptools._distutils.sysconfig import get_config_var |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
@@ -122,7 +122,7 @@ def get_sdk_level(): | |||
assert sdkname.startswith("MacOSX") | |||
assert sdkname.endswith(".sdk") | |||
|
|||
if sdkname == "MacOSX.sdk": | |||
if sdkname == "MacOSX.sdk" or "." not in sdkname[6:-4]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
@@ -221,16 +221,10 @@ def build_project(project, extra_args): | |||
shutil.rmtree(os.path.join(proj_dir, "build")) | |||
|
|||
print("Installing {!r} using {!r}".format(project, sys.executable)) | |||
status = subprocess.call( | |||
subprocess.check_call( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this change, the old code nicely prints which subproject fails to install and the new one doesn't.
if not ok: | ||
break | ||
print(f"\nBuilding {project}...\n") | ||
build_project(project, sys.argv[1:]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not ok. The old code is intentional, the code in framework wrappers should either build correctly or be skipped entirely (that is, bindings for frameworks introduced in macOS 11 will be skipped automatically on macOS 10.15 or earlier).
from distutils.errors import DistutilsError, DistutilsPlatformError, DistutilsSetupError | ||
from distutils.sysconfig import get_config_var as _get_config_var | ||
from distutils.sysconfig import get_config_vars | ||
from setuptools._distutils import log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once again imports from a private submodule.
How does MacPorts build PyObjC? The "install.py" and "development.py" scripts in the root of the repository are mostly meant as easy ways to install the software during development, that they are useful for others is a nice side effect. The correct way to install PyObjC is to use the distribution artefacts on PyPI, where the wheel archives are the preferred installation archive. Although I totally understand that a project like MacPorts prefers to install from source. |
See my previous question: How does macports package pyobjc? The correct way to package the software is IMHO to package all subprojects, and not use install.py/develop.py. |
I won't be changing "-Werror" in the setup.py files, I've caught too many errors that would have been missed without this flag. Apparently some users get compile time errors in some situations, but so far I've not been able to reproduce these. |
In order to do that, I'd have to add logic to extract all the many frameworks, list each and every one as a subport and determine on which platforms each one is available. In other words, I'd have to reimplement the logic you already have in |
Could you file an issues about this with the features you need and would like to have for this? The main interface for installing pyobjc is to use "pip install pyobjc" using the artefacts on PyPI. That's obviously not entirely compatible with package managers like MacPorts. The install.py and develop.py scripts slowly evolve over time as my needs change, without knowing your requirements this is bound to result in accidentally breaking your tooling in the future :-( |
The PyObjC build system is currently quite lenient and fragile. In particular, failure to build a module will just stop the build, print something, and exit with success. This has caused issues for MacPorts, where we build PyObjC for all applicable OS releases, hosted on that release itself, and preferably using the same libffi everywhere.
The just-stop-and-feign-success behaviour became particularly annoying on Big Sur and Catalina, where
pyobjc-core
doesn't actually build with stock libffi. So, I fixed it to use the system libffi, and made it usesubprocess.check_call()
for invoking sub-builds, only to find out that anything earlier than 10.11 as well as 10.13 didn't build.This PR also contains some other fixes to use setuptools more, handle more SDKs and not pass
-Werror
during the build. Some of these might not be acceptable to you; if so, I can drop them.