-
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?
Changes from all commits
81ba485
6224254
79e6721
1ad85a4
ec8c4c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
TOPDIR = os.path.dirname(os.path.abspath(__file__)) | ||
|
||
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Because MacOSX11.sdk exists as a valid option at There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. db2cdc8 implements the new code. |
||
try: | ||
with open(os.path.join(sdk, "SDKSettings.plist"), "rb") as fp: | ||
pl = plistlib.load(fp) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. changeset 12daa01 removes the dependency on distutils in this file. |
||
|
||
import objc # noqa: F401 | ||
from AddressBook import * # noqa: F401, F403 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. See above |
||
|
||
TOPDIR = os.path.dirname(os.path.abspath(__file__)) | ||
|
||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. See above |
||
try: | ||
with open(os.path.join(sdk, "SDKSettings.plist"), "rb") as fp: | ||
pl = plistlib.load(fp) | ||
|
@@ -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 commentThe 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. |
||
[sys.executable, "setup.py", "install"] + extra_args, cwd=proj_dir | ||
) | ||
|
||
if status != 0: | ||
print("Installing {!r} failed (status {})".format(project, status)) | ||
return False | ||
|
||
return True | ||
|
||
|
||
def version_key(version): | ||
return tuple(int(x) for x in version.split(".")) | ||
|
@@ -242,9 +236,8 @@ def main(): | |
sys.exit(1) | ||
|
||
for project in ["pyobjc-core"] + sorted_framework_wrappers(): | ||
ok = build_project(project, sys.argv[1:]) | ||
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 commentThe 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). |
||
|
||
|
||
if __name__ == "__main__": | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,10 +9,14 @@ | |
import warnings | ||
from setuptools import Extension, setup | ||
from setuptools.command import build_ext, build_py, egg_info, install_lib, test | ||
from distutils import log | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. once again imports from a private submodule. |
||
from setuptools._distutils.errors import ( | ||
DistutilsError, | ||
DistutilsPlatformError, | ||
DistutilsSetupError, | ||
) | ||
from setuptools._distutils.sysconfig import get_config_var as _get_config_var | ||
from setuptools._distutils.sysconfig import get_config_vars | ||
|
||
from pkg_resources import add_activation_listener, normalize_path, require, working_set | ||
|
||
|
@@ -50,7 +54,7 @@ def get_sdk_level(sdk): | |
sdkname = os.path.basename(sdk) | ||
assert sdkname.startswith("MacOSX") | ||
assert sdkname.endswith(".sdk") | ||
if sdkname == "MacOSX.sdk": | ||
if sdkname == "MacOSX.sdk" or "." not in sdkname[6:-4]: | ||
try: | ||
with open(os.path.join(sdk, "SDKSettings.plist"), "rb") as fp: | ||
pl = plistlib.load(fp) | ||
|
@@ -80,7 +84,6 @@ def get_sdk_level(sdk): | |
"-Wshorten-64-to-32", | ||
# "-fsanitize=address", "-fsanitize=undefined", "-fno-sanitize=vptr", | ||
# "--analyze", | ||
"-Werror", | ||
"-I/usr/include/ffi", | ||
# "-fvisibility=hidden", | ||
# "-O3", "-flto", | ||
|
@@ -487,7 +490,7 @@ def _fixup_compiler(use_ccache): | |
|
||
|
||
class oc_build_ext(build_ext.build_ext): | ||
user_options = [ | ||
user_options = build_ext.build_ext.user_options + [ | ||
( | ||
"deployment-target=", | ||
None, | ||
|
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
.