[build] Remove SWIFT_SDK_${OS}_ARCH_${ARCH}_LIBC_ARCHITECTURE_INCLUDE_DIRECTORY CMake variable

[finagolfin]

This variable is no longer used on linux and WASI after #32404, and pulls have been submitted to remove its use in Android and OpenBSD, #35707 and #40557.

@MaxDesiatov, would you review and start the CI?

[MaxDesiatov]

@swift-ci please smoke test

[MaxDesiatov]

I'm not a code owner of this, other people will be more appropriate for reviewing.

[finagolfin]

@compnerd, should be a quick review.

[finagolfin]

Linux CI failure looks like a flaky test, unrelated to this pull.

[finagolfin]

Rebased and corrected variable name in commit message.

[compnerd]

I believe you are correct, this should be safe to clean up now. Thanks!

[compnerd]

@swift-ci please test

[MaxDesiatov]

@buttaface this actually causes issues in WASI builds since we're using LIBC_ARCH_INCLUDE_PATH. Would it be possible to revert this, at least partially?

[MaxDesiatov]

This variable is no longer used on linux and WASI after #32404

I'm not sure how this could be true, since #32404 doesn't mention WASI or touch any code related to WASI. WASI doesn't rely on Glibc, it's based on musl.

[finagolfin]

this actually causes issues in WASI builds since we're using LIBC_ARCH_INCLUDE_PATH

As in code that is not in this repo yet, but only in your fork right now?

I'm not sure how this could be true, since #32404 doesn't mention WASI or touch any code related to WASI. WASI doesn't rely on Glibc, it's based on musl.

I searched for all instances of the variable in this repo and removed them all. Since the glibc modulemap mentions WASI and there is no musl modulemap in this repo, I figured you were using the same glibc modulemap.

Can you explain how you're using this variable: are you unable to use a single header modulemap like all these other platforms are switching to? Are you actually setting an arch-dependent path now, unlike the line I removed from this repo, where it was identical to the arch-independent path set the line above?

Let me know what you need and we can get that in, but I think this pull is the right cleanup for most platforms.

[finagolfin]

Btw, one nit that I didn't notice till now: this pull does not touch any variable named LIBC_ARCH_INCLUDE_PATH, as there never was such a variable in this repo. 😉

[MaxDesiatov]

this actually causes issues in WASI builds since we're using LIBC_ARCH_INCLUDE_PATH

As in code that is not in this repo yet, but only in your fork right now?

Code in this repo from this PR was propagated to our fork and broke SwiftWasm builds.

I'm not sure how this could be true, since #32404 doesn't mention WASI or touch any code related to WASI. WASI doesn't rely on Glibc, it's based on musl.

I searched for all instances of the variable in this repo and removed them all. Since the glibc modulemap mentions WASI and there is no musl modulemap in this repo, I figured you were using the same glibc modulemap.

There's no musl modulemap, but there's a separate WASILibc modulemap based on musl.

Can you explain how you're using this variable: are you unable to use a single header modulemap like all these other platforms are switching to? Are you actually setting an arch-dependent path now, unlike the line I removed from this repo, where it was identical to the arch-independent path set the line above?

SwiftWasm is currently not using a single header modulemap and switching to such modulemap causes build issues for us.

Let me know what you need and we can get that in, but I think this pull is the right cleanup for most platforms.

I would hope that cleanups that seem right for most platforms are not made at the expense of other platforms.

Btw, one nit that I didn't notice till now: this pull does not touch any variable named LIBC_ARCH_INCLUDE_PATH, as there never was such a variable in this repo. 😉

This variable is set based on the value of SWIFT_SDK_WASI_ARCH_wasm32_LIBC_ARCHITECTURE_INCLUDE_DIRECTORY that was removed here in this PR. Reverting this PR fixes the build issues for us.

[finagolfin]

Code in this repo from this PR was propagated to our fork and broke SwiftWasm builds.

I hope you're not expecting me to not break forks, 😉 I did ask you to review this pull when I submitted it.

There's no musl modulemap, but there's a separate WASILibc modulemap based on musl.

OK, but not in this repo either.

SwiftWasm is currently not using a single header modulemap and switching to such modulemap causes build issues for us.

Got it, Bionic was in the same boat until recently.

I would hope that cleanups that seem right for most platforms are not made at the expense of other platforms.

It is not clear that it is at your expense, see below.

This variable is set based on the value of SWIFT_SDK_WASI_ARCH_wasm32_LIBC_ARCHITECTURE_INCLUDE_DIRECTORY that was removed here in this PR.

As I noted above, that variable uses an identical path to the architecture-independent path SWIFT_SDK_WASI_ARCH_wasm32_LIBC_INCLUDE_DIRECTORY just above it in this repo. Since your code using the removed variable isn't in this repo, couldn't you just modify your extra modulemap generation to use this architecture-independent path instead? Or does your WASI modulemap actually vary by architecture, as I asked you above?

[MaxDesiatov]

I hope you're not expecting me to not break forks

Whether or not platform support is maintained in a fork is not relevant. In fact, as there's no way to easily set up CI for other platforms on this repo, the only way to develop platform support is in a fork. Unless you're a member of the Swift team fully having access to this repository and Swift CI. Developers outside Apple would inevitably create a fork just to submit a change here.

I'm talking specifically about platform support here, maintaining which is already hard enough as it is. Of course, not breaking any arbitrary Swift fork in the world is an unreasonable expectation, I'm not suggesting that.

I would like to take extra care not breaking Android or other platforms when submitting my PRs, I was hoping this could be mutual.

[finagolfin]

Whether or not platform support is maintained in a fork is not relevant.

I disagree, committers to this repo cannot be expected to know or care what is going on with all the random forks out there. The best we can do is ask people who have forked for review if we've heard of them, which I did.

Android even has a community CI whose build status is on the front page of this github repo, and it breaks all the time. Unless you add your platform to the official PR CI here instead, as Windows was recently included, I fully expect forks to break all the time.

Rather than bickering about process, which I hope I've made clear I think you're wrong about, would you answer my question of whether the simple fix I suggested for your off-repo code would fix your build problem?

[MaxDesiatov]

or care what is going on with all the random forks out there.

I've clarified above, I'm not talking about random forks. I'm referring to actively maintained forks dedicated to making Swift available on more platforms.

whether the simple fix I suggested for your off-repo code would fix your build problem?

I'm not sure what fix you're referring to. The simplest fix was to revert this change, which I did in our fork, and which unfortunately increases the diff between the fork and this repo and the likelihood of future conflicts.

I don't think you should assume that all platforms can have a single header libc. In my opinion, support for these include paths should stay in this repo until all platforms can migrate to a single header libc or find a better way to maintain these paths. I'm not asking you not to use single header libc on Android or any other platform, I only would like the existing WASI support not to be removed.

[finagolfin]

I've clarified above, I'm not talking about random forks. I'm referring to actively maintained forks dedicated to making Swift available on more platforms.

Understood, I saw your subsequent edit of your previous comment. I am calling anything not in this official repo a "random fork:" the committers here cannot be expected to check other forks, even "actively maintained" platform forks, to see what might break if they change something here. It is hard enough to figure that out for what is in this official repo.

I'm not sure what fix you're referring to.

I noted twice above that the architecture-independent SWIFT_SDK_WASI_ARCH_wasm32_LIBC_INCLUDE_DIRECTORY is set to the same path as the removed architecture-dependent variable that you want in this repo, and unless your musl-based libc specifically places architecture-dependent headers in different paths, only using that remaining architecture-independent path should suffice. I was ignored both times.

I don't think you should assume that all platforms can have a single header libc.

I am not, rather I'm assuming that this architecture-dependent variable that I removed is not needed since the platform it was added for, linux in #282, no longer uses it. I asked repeatedly if WASI actually uses it in your fork, since it is set identically to the still available SWIFT_SDK_WASI_ARCH_wasm32_LIBC_INCLUDE_DIRECTORY in this repo, and have not received a response from you.

I suggest we find a way to fix the problems you're having by looking at that, rather than wasting time on discussing the commit process for this repo, which whether you like it or not operates the way I describe, not the way you would like.