Use .so as the default dynamic library extension on unknown OSes
#7417
Conversation
LLVM defaults to `.so` for unknown OSes, so it makes sense to do the same in SwiftPM. `fatalError` here can lead to a crash while loading a package manifest containing `.library` products with none-OS targets even though the product won't be used in the build.
|
Out of curiosity what OS were you using which hit this code path |
|
I used armv7em-none-none-eabi. |
| @@ -147,7 +147,7 @@ extension Triple { | |||
| case .wasi: | |||
| return ".wasm" | |||
| default: | |||
| fatalError("Cannot create dynamic libraries for os \"\(os)\".") | |||
There was a problem hiding this comment.
Can we emit a diagnostic message as a warning here instead?
There was a problem hiding this comment.
Sounds resonable to me
There was a problem hiding this comment.
On second thought, I think emitting diagnostic here does not help users because users usually don't have any actionable fixes to suppress such warnings.
So I'd like to keep this as is, WDYT?
There was a problem hiding this comment.
If it's an unknown OS, an actionable fix is to submit a feature request on SwiftPM to add support for such OS. Otherwise it seems wrong that we're silently using a file extension that can be incorrect without giving users any clues as to what could be wrong.
There was a problem hiding this comment.
If you don't want to see this warning for armv7em-none-none-eabi, I think we should distinguish here none and triples that we just don't know how to handle and stick to .so in anticipation that could work (supposedly).
There was a problem hiding this comment.
I was thinking of providing ways to explicitly configure their expected file extension as an escape hatch via environment variable or configuration file.
e.g. SWIFTPM_TRIPLE_ARMV8EM_NONE_NONE_EABI_DYLIBEXT=.so or
// ./.swiftpm/configuration/target_info.json
{
"triples": {
"armv7em-none-none-eabi": {
"dynamicLibraryExtension": ".so"
}
}
}
There was a problem hiding this comment.
Full triple instead of platform because we expect it may change between the specified OS?
I'm not sure this is really the right place for the warning anyway though. If a platform is unsupported, then it seems likely we'd fail elsewhere. IMO we should either decide that we should have a warning for that case in general (outside of this particular call) with the escape hatch for known-customizable cases, or not diagnose.
There was a problem hiding this comment.
Full triple instead of platform because we expect it may change between the specified OS?
Exactly. e.g. only none OS information does not help decide the extension.
IMO we should either decide that we should have a warning for that case in general (outside of this particular call) with the escape hatch for known-customizable cases, or not diagnose.
Yeah, totally agree. Having a warning and adding such escape hatch at the same time sounds like the right way to go to me because such flexibility would be quite beneficial for Embedded scenarios. (BTW Rust toolchain allows developers to have flexible target specification and we can learn from them)
There was a problem hiding this comment.
Fair enough, then this can go in as is unless Ben has any objections, and it needs a test.
There was a problem hiding this comment.
The isDarwin check should be changed to isApple. <arch>-apple-<os> should be almost always be a dylib
MaxDesiatov
added
needs tests
Use
.soas the default dynamic library extension on unknown OSesMotivation:
fatalErrorhere can lead to a crash while loading a package manifest containing.libraryproducts with none-OS targets even though the product won't be used in the build.Modifications:
Use
.soas the default dynamic library extension on unknown OSesLLVM defaults to
.sofor unknown OSes, so it makes sense to do the same in SwiftPM.Result:
Dynamic libraries for Embedded targets will be suffixed with
.so