Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use .so as the default dynamic library extension on unknown OSes #7417

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Use .so as the default dynamic library extension on unknown OSes #7417

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/Basics/Triple+Basics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ extension Triple {
case .wasi:
return ".wasm"
default:
fatalError("Cannot create dynamic libraries for os \"\(os)\".")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we emit a diagnostic message as a warning here instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds resonable to me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@MaxDesiatov MaxDesiatov Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

@kateinoigakukun kateinoigakukun Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
    }
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@kateinoigakukun kateinoigakukun Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, then this can go in as is unless Ben has any objections, and it needs a test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isDarwin check should be changed to isApple. <arch>-apple-<os> should be almost always be a dylib

return ".so"
}
}

Expand Down