Skip to content

Conversation

@ronaldoussoren
Copy link
Contributor

@ronaldoussoren ronaldoussoren commented Feb 20, 2024

Updates to the build system to support fat builds (--enable-universalsdk) on macOS with the experimental JIT.

Current status: "it compiles, therefore it works"

@bedevere-app bedevere-app bot mentioned this pull request Feb 20, 2024
o = tempdir / f"{opname}.o"
args = [
f"--target={self.triple}",
"-isysroot", "/Users/ronald/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be removed in the final version, and likely needs a different issue and patch: I installed llvm using a download on llvm's GitHub, and use Xcode to build python (not Command Line Tools). Without this addition "clang-16" cannot find the SDK to use.

):
return
stencil_groups = asyncio.run(self._build_stencils())
stencil_groups = ASYNCIO_RUNNER.run(self._build_stencils())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is the right way to enable running multiple builds, I'm a very light asyncio user. Without this change the second run fails due to running with a different runloop.

args.target.build(pathlib.Path.cwd(), comment=comment)

else:
# Multiple triples specified, assume this is a macOS multiarchitecture build
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit is not ideal, multiple files are created and that likely breaks dependency handling in the Makefile.

It should be possible to publish everything in a single header file, but that requires more significant changes to the builder.

with open("jit_stencils.h", "w") as fp:
for idx, target in enumerate(args.target):
cpu, _, _ = target.triple.partition("-")
fp.write(f"#{'if' if idx == 0 else 'elif'} defined(__{cpu}__)\n")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for x86_64 and arm64, but might break for ancient systems running PPC and PPC64. I don't care for those.

@brandtbucher
Copy link
Member

Thanks for working on this! Juggling a few tasks right now, but I'll try this out later this week.

@brandtbucher
Copy link
Member

Something like this fixes CI for me, and generalizes this to all platforms:

diff --git a/Tools/jit/_targets.py b/Tools/jit/_targets.py
index 37b3e596bd..fd82eda697 100644
--- a/Tools/jit/_targets.py
+++ b/Tools/jit/_targets.py
@@ -37,6 +37,7 @@
 @dataclasses.dataclass
 class _Target(typing.Generic[_S, _R]):
     triple: str
+    condition: str
     _: dataclasses.KW_ONLY
     alignment: int = 1
     prefix: str = ""
@@ -108,7 +109,6 @@ async def _compile(
         o = tempdir / f"{opname}.o"
         args = [
             f"--target={self.triple}",
-            "-isysroot", "/Users/ronald/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk",
             "-DPy_BUILD_CORE",
             "-D_DEBUG" if self.debug else "-DNDEBUG",
             f"-D_JIT_OPCODE={opname}",
@@ -383,15 +383,21 @@ def _handle_relocation(
 def get_target(host: str) -> _COFF | _ELF | _MachO:
     """Build a _Target for the given host "triple" and options."""
     if re.fullmatch(r"aarch64-apple-darwin.*", host):
-        return _MachO(host, alignment=8, prefix="_")
+        condition = "defined(__aarch64__) && defined(__APPLE__)"
+        return _MachO(host, condition, alignment=8, prefix="_")
     if re.fullmatch(r"aarch64-.*-linux-gnu", host):
-        return _ELF(host, alignment=8)
+        condition = "defined(__aarch64__) && defined(__linux__)"
+        return _ELF(host, condition, alignment=8)
     if re.fullmatch(r"i686-pc-windows-msvc", host):
-        return _COFF(host, prefix="_")
+        condition = "defined(_M_IX86)"
+        return _COFF(host, condition, prefix="_")
     if re.fullmatch(r"x86_64-apple-darwin.*", host):
-        return _MachO(host, prefix="_")
+        condition = "defined(__x86_64__) && defined(__APPLE__)"
+        return _MachO(host, condition, prefix="_")
     if re.fullmatch(r"x86_64-pc-windows-msvc", host):
-        return _COFF(host)
+        condition = "defined(_M_X64)"
+        return _COFF(host, condition)
     if re.fullmatch(r"x86_64-.*-linux-gnu", host):
-        return _ELF(host)
+        condition = "defined(__x86_64__) && defined(__linux__)"
+        return _ELF(host, condition)
     raise ValueError(host)
diff --git a/Tools/jit/build.py b/Tools/jit/build.py
index 800d8e31b0..7872004a90 100644
--- a/Tools/jit/build.py
+++ b/Tools/jit/build.py
@@ -42,8 +42,7 @@
 
         with open("jit_stencils.h", "w") as fp:
             for idx, target in enumerate(args.target):
-                cpu, _, _ = target.triple.partition("-")
-                fp.write(f"#{'if' if idx == 0 else 'elif'} defined(__{cpu}__)\n")
+                fp.write(f"#{'if' if idx == 0 else 'elif'} {target.condition}\n")
                 fp.write(f'#   include "jit_stencils-{target.triple}.h"\n')
 
             fp.write("#else\n")

@savannahostrowski
Copy link
Member

@ronaldoussoren Hey there! Are you planning on carrying this forward? I have the branch pulled down locally and have patched things up with the main, etc. I'm happy to carry it forward if you're working on other things. Just let me know!

@ned-deily
Copy link
Member

While preparing for the 3.14.0a2 release, I realized too late that this issue had not yet been resolved thus preventing supporting the JIT in the python.org macOS installer build. My apologies for that. I'm looking at the issue now. If anyone does have any updates for it, it would be great to get them into the draft PR soon so we can re-target this for 3.14.0a3.

@ned-deily ned-deily self-requested a review November 19, 2024 17:20
@ronaldoussoren
Copy link
Contributor Author

@ronaldoussoren Hey there! Are you planning on carrying this forward? I have the branch pulled down locally and have patched things up with the main, etc. I'm happy to carry it forward if you're working on other things. Just let me know!

Feel free to carry it forward. I'm afraid I won't have time to work on this until at least the end of the year.

@brandtbucher
Copy link
Member

The other PR was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants