PackageModel: partially address #5719 #5981
Conversation
compnerd
requested review from
abertelrud,
elsh,
neonichu and
tomerd
as code owners
compnerd
force-pushed
the
5712
branch
2 times, most recently
from
c25ed88 to
6986404
Compare
| // TODO(5719) use `lld-link` if the build requests lld. | ||
| // TODO(5719) handle `-Xmanifest` vs `-Xswiftc`. | ||
| // `-use-ld=` is always joined in Swift. | ||
| if let ld = extraSwiftFlags.first(where: { $0.starts(with: "-use-ld=") }) { |
There was a problem hiding this comment.
Seems a bit unfortunate to have to parse the flags like this, can/should we ask users to configure alternative linkers more explicitly?
There was a problem hiding this comment.
I agree it is unfortunate. How would you recommend that we set that up? The general mechanism for this is -use-ld=... which with CMake you would pass via CMAKE_Swift_FLAGS and that is honoured everywhere.
There was a problem hiding this comment.
if we have to parse, could we use argument parser to help make this more reliable?
There was a problem hiding this comment.
I'm not sure if that is going to be any better because we will need to consider the case of -Xswiftc or -Xclang-linker where the option itself is made opaque. We would need to process both the driver and frontend flags and possibly non-swift-tool flags.
There was a problem hiding this comment.
this does seems very brittle. any other ideas on how to make it less so? argument parser was the only one I could think of.
|
can we add a test? |
|
BTW, I think that this might be something we want to pull into 5.8 |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test Linux platform |
|
@swift-ci please smoke test |
Done |
|
@swift-ci please smoke test |
compnerd
changed the title
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
| return librarian.basename | ||
| } | ||
| // TODO(5719) use `lld-link` if the build requests lld. | ||
| // TODO(5719) handle `-Xmanifest` vs `-Xswiftc` |
There was a problem hiding this comment.
Yes, it is, we still need to refine it further for more ways that the flags can be specified.
| // TODO(5719) use `lld-link` if the build requests lld. | ||
| // TODO(5719) handle `-Xmanifest` vs `-Xswiftc` | ||
| // `-use-ld=` is always joined in Swift. | ||
| if let ld = extraSwiftFlags.first(where: { $0.starts(with: "-use-ld=") }) { |
There was a problem hiding this comment.
string parsing seems fragile, is there a better way?
There was a problem hiding this comment.
I really don't see a better way unfortunately. The driver flags, frontend flags, and non-Swift tool flags are all intertwined and will impact this. I think that this might be the best approach (at least for now).
There was a problem hiding this comment.
d77f51d#diff-adf7999cea9f76ad6a7d320c04fbfa7222d4c7d20bd27f81667c02cf88081897R140 BTW is where this becomes Windows specific.
|
@swift-ci please smoke test |
|
would rather see a more idiomatic solution, but fine with this change if it only impact building on windows. |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
Handle `-use-ld=` from the configuration in the selection of the librarian. We do not honour the `-Xmanifest` or `-Xswiftc` flags being passed on the command line rather than the plist as we cannot pre-compute the flags.
|
@swift-ci please smoke test |
4 participants
Handle
-use-ld=from the configuration in the selection of the librarian. We do not honour the-Xmanifestor-Xswiftcflags being passed on the command line rather than the plist as we cannot pre-compute the flags.