[build-script] Add the installation prefix to the toolchain path #30565
Conversation
In several places, it was assumed to be /usr and hard-coded to that. This makes sure the installation prefix actually passed in is used.
|
After looking into this more, I realized that |
| @@ -284,5 +284,7 @@ def toolchain_path(install_destdir, install_prefix): | |||
| built_toolchain_path = install_destdir | |||
| if platform.system() == 'Darwin': | |||
| # The prefix is an absolute path, so concatenate without os.path. | |||
| built_toolchain_path += darwin_toolchain_prefix(install_prefix) | |||
| built_toolchain_path += darwin_toolchain_prefix(install_prefix) + "/usr" | |||
There was a problem hiding this comment.
I'm not sure if calling this a "toolchain path" is good terminology, since on Darwin that would mean the .xctoolchain directory. That's also what the documentation string for this method says. After this change, it's always an absolute form of the install_prefix. I'm not sure what to call that 🤔
By the way, darwin_toolchain_prefix removes usr from install_prefix, and then this adds it back.
There was a problem hiding this comment.
I'm not sure if calling this a "toolchain path" is good terminology
I have no idea why this is needed on Darwin or what the common paths are there, I just tried to keep the behavior the same.
By the way, darwin_toolchain_prefix removes usr from install_prefix, and then this adds it back.
Yes, as I noted in my previous comment, that is what was being done everywhere already, so rather than adding it back in a dozen different places, I simply added it back once. 😄
I figured it might be because install_prefix on Darwin doesn't necessarily end with /usr, so darwin_toolchain_prefix() just removes that last component and the rest of the code replaces it with /usr. But that's just a guess, given my professed ignorance of the Darwin toolchain.
Just let me know what should be done, as this pull actually starts using install_prefix on non-Darwin platforms, rather than assuming it's always /usr.
|
I missed one, swiftlang/sourcekit-lsp#257 also submitted now. |
|
Build failed |
|
The mac/Windows failures seem spurious, linux works. |
|
Alright, looks like everything passes on the CI, barring the spurious Windows failure. Feel free to change the docs or let me know what you want changed for the comments. |
|
@edymtt Can you please review this? |
|
Linux Toolchain (Ubuntu 16.04) Install command |
|
macOS Toolchain Install command |
|
Ping, would be good to get this in before the 5.3 branch, as setting |
|
cc: @najacque |
|
I had to use a portion of this patch to get SwiftPM to build in the package script for Swift 5.2 that just got accepted into the Termux app for Android. @drexin, this pull may also be worth merging into the 5.2 branch, or at least the parts of it that apply. |
|
@benlangmuir, you approved some of these simple pulls, mind reviewing the rest? |
|
@swift-ci test windows |
|
Passes all CI, ready to go? |
|
@buttaface sorry things are moving slowly here. This hasn't been forgotten, but the folks who have the expertise to review this have been busy. @shahmishal are you still planning to look at this soon? |
|
Thanks @buttaface! |
|
Thanks for reviewing and merging, one less patch I have to keep locally. |
In several places, it was assumed to be /usr and hard-coded to that. This makes sure the installation prefix actually passed in is used.
I just noticed this when packaging a recent Swift 5.2 snapshot for Android, as I use
/data/data/com.termux/files/usras the install prefix there, not the usual/usr. There are three additional instances where(install_)toolchain_path()is passed without the install prefix, so those repos are likely hard-coding/usrthemselves and will need to be fixed.If someone gives me the go-ahead on this pull, I will submit pulls for those other repos or contact their developers. @aciidb0mb3r, you added the SwiftPM part of this pull, which is what I actually hit, so let me know what you think. @ahoppen, you added some of these.