Proposal for scarthgap branch (Swift 6.1.2) #12
Conversation
|
I can separate out further if required, hopefully this makes it easier to cherry-pick / learn about Yocto etc |
lhoward
force-pushed
the
release/6.1_separate_commits
branch
from
b05bc90 to
a8457aa
Compare
|
I'll reword commits to be a bit more loquacious when I've a moment |
|
@lhoward @colemancda @compenguy I do not like the idea of checking in all these cmake files. Those should be provided by a sysroot package. Not copying them to the workdir. That is not maintainable. |
|
Sure. But they definitely shouldn't come from an unstable URL either. They should come from the SSOT upstream projects or we should build Swift from source. Isolating the change to this commit should hopefully permit the other changes to be reviewed in parallel. |
|
We can pull them from Apple's llvm-project fork but likely we'll need to build LLVM to expand the macro substitutions. I had hoped to defer this until we actually build Swift from source entirely. |
I've found with the swift-armv7 scripts that it's possible to simply grab llvm-project, configure it with cmake without actually building it, and point Swift to the cmake configure folder and it works. If you like I can have a look at this on this branch for you since I've already done it, and it should be somewhat trivial to do this with Yocto! llvm-project gets the same tags as Swift libraries, so we can use the same swift-6.1.2-RELEASE tag to have a consistent version of LLVM against Swift ✌🏻 |
|
llvm-project gets the same tags as Swift libraries, so we can use the same swift-6.1.2-RELEASE tag to have a consistent version of LLVM against Swift ✌🏻 |
Great, shall I give you write access to the branch? I actually started doing this last night but ran out of time. |
|
Or just pop the recipe in here as an attachment and I'll merge and rebase. |
|
@lhoward I was going to PR to this branch for you so we can review the changes from one to the other. Then you can merge it in. I will note tho- this branch fails to build correctly for armv7. There is a typo in do_fix_gcc_install_dir, building the embedded stdlib needs to be turned off, CxxInterop doesn't seem to be building, and also Foundation is compiling in a weird way and creating an unknown target triple. Shall we try to fix these as a part of this set of changes, or can this be done separately? |
|
I'll take whatever you have, whenever you have it :) |
|
Here is the PR: PADL#1 I just need your help confirming it does build correctly, if you have a moment |
|
Onto it! |
lhoward
force-pushed
the
release/6.1_separate_commits
branch
2 times, most recently
from
0f94c31 to
2effd4d
Compare
Fixed (thanks to @xtremekforever) |
lhoward
force-pushed
the
release/6.1_separate_commits
branch
5 times, most recently
from
524483f to
2ac6389
Compare
I didn't notice an issue with C++ interop. With a couple of small changes, this branch is now building for armv7. I believe the only remaining issues for parity with masters are CI and dunfell support. I suggest we only support scarthgap for Swift 6.x unless someone else wants to put in the work. CI is revised for Scarthgap/GH runner, just need to wait for it to complete. |
lhoward
force-pushed
the
release/6.1_separate_commits
branch
4 times, most recently
from
03fc213 to
9075604
Compare
|
CI appears to be ok. Propose this could be the basis of a scarthgap branch (in which case let's remove in-tree CMake). |
|
@lhoward let me try to build this for beaglebone Yocto on my end and confirm it runs okay. If so then we should be good to go with this branch. |
|
That'll be the best for now, to remove the time mods here, then resolve the issues separately. I guess it's an issue for all 32-bit platforms! I did see that WASI has some special handling in NIO (and WASI is 32-bit) but idk if they are using 64-bit time there. Maybe worth checking |
I would prefer the layer consumer to patch SwiftNIO, even if it's a bit annoying, as changing the time_t ABI from the Yocto default may cause difficult to diagnose issues at runtime. For example, if a Swift recipe with 32-bit time_t uses a shared library with a 64-bit time_t API (that doesn't have a distinct symbol for 32-bit time_t clients like glibc does). |
So yes, I changed my mind :) |
|
Not sure why you'd want to make the layer unusable on 32-bit platforms without patching NIO and potentially other libraries as well...I understand that it would be nice and is more correct to do 64-bit time, but the full impact of switching to this in Swift is currently unknown... Could we reconsider including time_t upgrades here and tackle it separately? I will remind that Swift has been using 32-bit time on 32-bit platforms since this original layer at 5.7.1- so making this change now is an extra thing that now breaks armv7 compatibility until a bunch of things get patched. |
|
Skimming through the rust libc thread about dealing with time_t on 32-bit systems, it looks like glibc's C headers can be used with preprocessor defines to direct you to the 32-bit versions or the 64-bit versions of the time types and functions. Alternatively, you can be explicit (e.g. calling __clock_gettime64 directly). |
|
IMO it should then be disabled distro wide to avoid unexpected ABI issues. Essentially the implication is that some of your image is built with one ABI, and the rest, another. But, happy to disable with a loud bbwarn that your program may crash. |
|
I will set armv7 isn't an important platform to me (at the moment, at least), so I'm happy to accomodate consensus here ;) |
This is correct. The issue is that Yocto (sensibly, if you ask me), enables 64-bit timestamps on 32-bit systems by default, because typically you're building the entire image so you don't need to worry about ABI compatibility with external packages – it's effectively a closed system. |
This overrides the Yocto default and may cause your program to crash if Swift packages link against other packages that do not have this disabled. Caveat emptor.
lhoward
force-pushed
the
release/6.1_separate_commits
branch
from
6b7f955 to
e968883
Compare
@jeremy-prater I'll leave it to you whether you think 64-bit timestamps on 32-bit platforms should be disabled unconditionally when building Swift recipes, or if it should be left to the end-user to do (with a suitable note in README.md). If you decide the latter than I will update before you merge; otherwise, good to go. |
in the interest of DRY, consolidate common variables between CMake and SwiftPM classes, moving per-architecture tune flags into a new class (swift-platform-tune.bbclass)
replace swift-${SWIFT_VERSION}-RELEASE with ${SWIFT_TAG}; this should make it
easier to test pre-release versions of Swift
lhoward
force-pushed
the
release/6.1_separate_commits
branch
from
df82849 to
a6e7ac2
Compare
|
Okay, this looks good to me! Then, when the 64-bit time is supported in libraries (when compiling on 32-bit platforms) it can be enabled!! |
|
@lhoward @jeremy-prater looks like this runs just fine on armv7 and doesn't include the time_t change either. So, looks like it'll work! |
|
Looks like we're ready to merge. One final question, would this layer support |
|
@jeremy-prater I think there's a problem building this on Kirkstone that @compenguy fixed in a tarball he sent earlier. So this layer is currently only compatible with Scarthgap and newer I believe. |
I don't think the issue here is syntax compatibility, but rather recipe version compatibility, e.g. cmake. |
I think there's value in getting this merged as-is, and if there's some work that can be done to make it simultaneously compatible with kirkstone, that can be a subsequent PR. But I think it's important to merged what we've got already as a milestone or checkpoint for basing all future work on. |
|
Sounds good, now do we want to squash merge, or just merge 38 commits? |
|
I vote for squash merge since we've had a good look at the commits!! |
|
I vote for keeping the commits. |
|
The commit messages have valuable context, for example re: 64-bit timestamps. But if you want to squash, it’s your choice. Squashing will also make it slightly harder to restore CMake for kirkstone. |
Exclusivity checking has high runtime performance cost, which is undesirable for embedded systems. Specifying both options doesn't make sense; opt for -enforce-exclusivity=unchecked which retains static analysis but disables runtime checking. [1] [1] https://www.swift.org/blog/swift-5-exclusivity/
export the Swift package resolution function as do_package_resolve so it can be used conditionally from recipes
Rework of #9 to separate changes into distinct commits.