wabiverse / SwiftUSD

Pixar's universal scene description for swift and the open source metaverse.
https://wabiverse.github.io/SwiftUSD/documentation/pixarusd/
Other
66 stars 5 forks source link

Add new `linkingStrategy` parameter to `.target()` dependencies. #17

Open furby-tm opened 1 month ago

furby-tm commented 1 month ago

Allow users to specify dynamic library targets in same-package dependencies, which are otherwise always statically linked with the following client-facing API changes to SwiftPM:

products: [
  .library(
    name: "DynamicLib",
    type: .dynamic,
    targets: ["DynamicLib"]
  )
]

// ok, dynamically linking DynamicLib.
.target(name: "DynamicLib", linkingStrategy: .matchProduct)

// ok, statically linking DynamicLib.
.target(name: "DynamicLib", linkingStrategy: .alwaysStatic)

// ok, falling back to default behavior.
.target(name: "DynamicLib")
furby-tm commented 1 month ago

cc. @stackotter for review:

https://github.com/wabiverse/SwiftPM/commit/5dc346a81bdeae2835d58b82906d10bd525d52fa

furby-tm commented 1 month ago

@stackotter after further review, is type: .dynamic along with a .alwaysStatic linking strategy something we want to be doing here?

My C/C++ senses tell me this is a weird thing to do, but given this is the historical default behavior perhaps it makes sense in this context?

Perhaps a tiny grammatical change we can make is to replace .alwaysStatic with .default here which simply fallbacks to the historical behavior of always statically linking (the same behavior that .alwaysStatic is intended to infer), but I suppose a .default option is less clear than having users explicitly declare .alwaysStatic:

// ok, statically linking DynamicLib.
.target(name: "DynamicLib", linkingStrategy: .default)
furby-tm commented 1 month ago

Pending #18 to be fixed before we can consider this as completed.

Edit: This bug is unrelated, it's a regression in SwiftPM main: https://github.com/swiftlang/swift-package-manager/issues/8055

stackotter commented 1 month ago

@stackotter after further review, is type: .dynamic along with a .alwaysStatic linking strategy something we want to be doing here?

My C/C++ senses tell me this is a weird thing to do, but given this is the historical default behavior perhaps it makes sense in this context?

Perhaps a tiny grammatical change we can make is to replace .alwaysStatic with .default here which simply fallbacks to the historical behavior of always statically linking (the same behavior that .alwaysStatic is intended to infer), but I suppose a .default option is less clear than having users explicitly declare .alwaysStatic:

// ok, statically linking DynamicLib.
.target(name: "DynamicLib", linkingStrategy: .default)

I think targets shouldn't be able to override the library type of products they depend on. So if a library specifically has type: .dynamic then it should always be compiled to a dylib whenever it's needed.

With the context that linkingStrategy is meant to control how the product links against the target, rather than the linking of the parent product, .alwaysStatic seems clear. So perhaps we should try to find a way to make that connotation of linkingStrategy more clear through naming or something.