Skip to content

The stdlib build currently only allows setting a single threading library type, even for multiple stdlibs #59568

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

Closed
finagolfin opened this issue Jun 18, 2022 · 8 comments
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior.

Comments

@finagolfin
Copy link
Member

Describe the bug
Since #59287, it is possible to set a threading library for the Swift stdlib with SWIFT_THREADING_PACKAGE, which is set to the host platform by default. However, the current config builds all stdlibs with a single type of threading library, which can be overridden with the flag --swift-threading-package but still applies to every stdlib. Since it's possible to cross-compile multiple non-host stdlibs, this won't work if any of them need a different threading library, though it's fine if they're all Darwin platforms with the same type of threading library, say macOS and iOS.

To Reproduce

  1. Build multiple stdlibs that require different threading libraries, and watch it usually fail when all are built against the host threading library type.

Expected behavior
You should be able to build multiple stdlibs against different types of threading libraries.

Environment:

  • OS: Ubuntu 20.04 x86_64

Additional context
I just ran into this because the Android armv7 stdlib built fine on the community Android armv7 CI, but failed on my Android CI. I dug into it and it's because the host SDK on the community CI is LINUX so it applies that threading library to Android armv7 too, based on the default logic linked above. My own Android CI doesn't build for the linux host though, only for Android armv7 with the flags --cross-compile-hosts=android-armv7 --skip-local-build, so the host default logic chooses pthreads for the Android host and fails for armv7 only, as pthreads doesn't support 32-bit here.

In this case, it so happens that this issue is getting the Android build to pass, but in general, it's not a good idea to build all stdlibs against only one type of threading library.

@finagolfin finagolfin added the bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. label Jun 18, 2022
@al45tair al45tair self-assigned this Jun 20, 2022
@al45tair
Copy link
Contributor

I was a little surprised that I hadn't dealt with this already because I thought I remembered looking at the CMake code and deciding that we needed to do something about cross compile builds where multiple stdlibs were involved. It turns out that I did intend for this to work, but got a couple of details wrong — #59589 should hopefully fix it. I'd be very grateful if you could try it with your Android armv7 test case and make sure it works for you. If it doesn't, then there's clearly more wrong than I think.

@finagolfin
Copy link
Member Author

I don't think that pull is going to fix it, as you're still only applying a single threading package to all stdlibs. I have a couple questions:

  1. You have some logic that checks if the threading package variable is an empty string, when would that be the case? Since that variable is always default-initialized, I don't think it will ever be empty, unless someone manually hardcodes it.
  2. It appears you use that same variable for the host and the target stdlibs, as both call threading_package_name(), which just uses the host-initialized SWIFT_THREADING_PACKAGE. How is that supposed to work for cross-compilation, are you expecting that SWIFT_THREADING_PACKAGE variable to be set to the empty string in that case?
  3. Do you intend for the flag --swift-threading-package to also apply for the host and all targets? Because that is what it does now.

I'm unclear on some of this thread library selection logic, please let me know your intent.

@al45tair
Copy link
Contributor

So --swift-threading-package as a top-level override is supposed to apply to everything you're building at the time, but I had intended that if you don't set it, then it will choose on a per-SDK basis, which should be what we want. However, I think you're right — the top level CMakeLists.txt sets it if it's empty, which I think is wrong.

@al45tair
Copy link
Contributor

al45tair commented Jun 20, 2022

There is another variable I'd added, SWIFT_SDK_${SDK}_THREADING_PACKAGE that you can explicitly set, but that only gets used by the tests at present. Maybe that should also be used by the stdlib build, since it seems not entirely improbable that you might want to override it for some particular SDK.

@al45tair
Copy link
Contributor

I'll update that PR with some more fixes once I've tested that they don't break everything.

@al45tair
Copy link
Contributor

Right. I've updated my PR if you'd like to take a look (but please review it there, rather than here, otherwise things just get confusing).

@finagolfin
Copy link
Member Author

So --swift-threading-package as a top-level override is supposed to apply to everything you're building at the time

I don't think that's a good idea, would be better to make it a list like --cross-compile-install-prefixes, but I'm not sure if it's possible to determine the order in which multiple stdlibs are built. If it is, I'd make it a list of threading library types, with the first one for the host, but it's fine if you don't want to go to that trouble yet.

Maybe that should also be used by the stdlib build

That would work for overriding cross-compilation for now.

The biggest issue is that your intended threading library auto-detect mode doesn't work currently, that is the main fix needed: the overrides are just nice to have.

@finagolfin
Copy link
Member Author

Confirmed that #59606 fixes this issue by trying it with various overrides, thanks for adding the list of overrides too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants