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.

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

Prefix linker flags with -Xlinker #7588

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

Conversation

rauhul
Copy link
Member

@rauhul rauhul commented May 24, 2024

Currently SwiftPM fowards unsafe flags under linker settings directly to swiftc un-prefixed. These flags should instead be prefixed with -Xlinker, swiftc flags can instead be passed as unsafe flags under Swift settings.

This commit updates SwiftPM to prefix unsafe linker flags with -Xlinker if the package the target is part of uses tools version 6.0.

Currently SwiftPM fowards unsafe flags under linker settings directly to
swiftc un-prefixed. These flags should instead be prefixed with
-Xlinker, swiftc flags can instead be passed as unsafe flags under Swift
settings.

This commit updates SwiftPM to prefix unsafe linker flags with -Xlinker
if the package the target is part of uses tools version 6.0.
@rauhul
Copy link
Member Author

rauhul commented May 24, 2024

Open for discussion!

@@ -390,7 +390,13 @@ public final class ProductBuildDescription: SPMBuildCore.ProductBuildDescription
// Other linker flags.
for target in self.staticTargets {
let scope = self.buildParameters.createScope(for: target)
flags += scope.evaluate(.OTHER_LDFLAGS)
let additionalFlags = scope.evaluate(.OTHER_LDFLAGS)
// FIXME: this should be based on the target package's tools version
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC our current convention was to base all behavior on root package's tools version? @neonichu do I understand that correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends. Mostly we try to apply the specific package's tools-version, but there are some behaviors that are more far reaching where we look at root packages.

I think for more build related things like this that should be localized to the package itself, looking at the specific package is the most appropriate.

@MaxDesiatov MaxDesiatov added the needs tests This change needs test coverage label Jun 5, 2024
@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Jun 5, 2024

Overall makes sense to me, but this needs tests and that FIXME to be cleaned up, either by clarifying or implementing the desired behavior.

@MaxDesiatov MaxDesiatov added the build system Changes to interactions with build systems label Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system Changes to interactions with build systems needs tests This change needs test coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants