Change how libraries are specified to the linker when using searched libs #890
Conversation
|
@swift-ci test |
daveinglis
force-pushed
the
building_tests_fixes_windows
branch
8 times, most recently
from
c224e9e to
843e629
Compare
daveinglis
changed the title
daveinglis
force-pushed
the
building_tests_fixes_windows
branch
5 times, most recently
from
e6674c3 to
3d422f3
Compare
|
@swift-ci test |
daveinglis
force-pushed
the
building_tests_fixes_windows
branch
from
3d422f3 to
27d13e5
Compare
|
@swift-ci test |
daveinglis
force-pushed
the
building_tests_fixes_windows
branch
from
27d13e5 to
4fe5921
Compare
|
@swift-ci test |
daveinglis
requested review from
aciidgh,
jakepetroules,
mhrawdon,
neonichu and
owenv
as code owners
| } | ||
|
|
||
| if specifier.useSearchPaths, basename.hasPrefix("lib"), basename.hasSuffix(".a") { | ||
| if specifier.useSearchPaths { |
There was a problem hiding this comment.
Should we emit some kind of warning here if the library name doesn't have one of the prefixes we think will allow it to be found via search paths?
| fileprivate extension LinkerSpec.LibrarySpecifier { | ||
| func searchPathFlagsForLd(_ name: String) -> [String] { | ||
| func searchPathFlagsForLd() -> [String] { | ||
| let strippedName: String |
There was a problem hiding this comment.
Same question here, should we warn if we get a filename format we don't recognize here / don't think will be found?
There was a problem hiding this comment.
Hmm, rethinking this.... I think we need to fallback to using absolute path linking when that happens, this would be the same as how it was before....
There was a problem hiding this comment.
There are also tests that would have this warning (and fail due to it), but since we will fallback to an absolute path, not sure the warning it necessary now.
There was a problem hiding this comment.
falling back w/ no warning makes sense to me
| self.dsymPath = dsymPath | ||
| self.xcframeworkSourcePath = xcframeworkSourcePath | ||
| self.privacyFile = privacyFile | ||
| self.libPrefix = prefixes.first |
There was a problem hiding this comment.
Why do we only consider the first element here?
There was a problem hiding this comment.
Ya, I was thinking about that and I just couldn't think of a case where there would ever be more that one so I forego the extract logic of handling multiple and just went with the lazy handle one method. Do you feel its worth handling multiple?
There was a problem hiding this comment.
Only handling one sounds ok to me, but I think we should then either only allow one in the spec or error out if we parse more than one
|
overall this looks great, just have a few questions |
|
we should also run this through cross-repo testing before merging |
daveinglis
force-pushed
the
building_tests_fixes_windows
branch
2 times, most recently
from
16c3a12 to
d788510
Compare
…libs - remove the platform specifics from computeLibraryArgs (we cannot assume that all libraries have a lib prefix and what there suffix is.) So we now use the FileType prefix and remove any suffix when using searchPathFlagsForLD, moving this into the LinkerSpec.LibrarySpecifier extension, this allows for proper searching of libraries, and linking of dynamic libraries (especially on Windows).
daveinglis
force-pushed
the
building_tests_fixes_windows
branch
from
d788510 to
cdcfe04
Compare
2 participants
that all libraries have a lib prefix and what there suffix is.) So we
now use the FileType prefix and remove any suffix when using
searchPathFlagsForLD, moving this into the LinkerSpec.LibrarySpecifier
extension, this allows for proper searching of libraries, and linking
of dynamic libraries (especially on Windows).