Skip to content

Forward --ld-path to linker driver #1416

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
Merged
merged 2 commits into from
Sep 5, 2023
Merged

Forward --ld-path to linker driver #1416

merged 2 commits into from
Sep 5, 2023

Conversation

kabiroberai
Copy link
Contributor

@kabiroberai kabiroberai commented Aug 16, 2023 •
edited
Loading

This PR introduces an --ld-path option, which is forwarded to the Clang linker-driver option with the same name. This change is required to support the toolset.linker.path option in SE-0387.

See also

Upstream: swiftlang/swift#67956.
Downstream: swiftlang/swift-package-manager#6719.

This was referenced Aug 16, 2023
Merged
Merged
@MaxDesiatov MaxDesiatov requested a review from artemcm August 16, 2023 10:20
@MaxDesiatov
Copy link
Contributor

@swift-ci test

compnerd
compnerd reviewed Aug 16, 2023
View reviewed changes
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I think that this should map to -B. Why is -tools-path insufficient?

@kabiroberai
Copy link
Contributor Author

@compnerd I think mapping -tools-path to -B would be a good follow-up, as that'd handle toolset.toolsetRootPath correctly. But the toolset schema allows each individual tool to have a custom path, so we still need to handle the possibility of toolset.linker.path being explicitly set.

@compnerd
Copy link
Member

@compnerd I think mapping -tools-path to -B would be a good follow-up, as that'd handle toolset.toolsetRootPath correctly. But the toolset schema allows each individual tool to have a custom path, so we still need to handle the possibility of toolset.linker.path being explicitly set.

Multiple -B options can be specified so that can still work. That said, if we are exposing that, can we make all of these options "invisible" as I'm not sure if we want them to be user visible - the toolset schema can coordinate with the driver with internal flags.

@artemcm
Copy link
Contributor

artemcm commented Aug 16, 2023

@swift-ci test

@kabiroberai
Copy link
Contributor Author

@compnerd fyi following up on your comment about -tools-directory, it looks like GenericUnixToolchain and WindowsToolchain indeed forward it to clang -B but this is missing from DarwinToolchain and WebAssemblyToolchain. Should I open a follow-up PR to patch this?

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Aug 16, 2023 •
edited
Loading

Patching those in a follow-up PR makes sense to me.

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov MaxDesiatov merged commit 4d7f6a0 into swiftlang:main Sep 5, 2023
MaxDesiatov added a commit to swiftlang/swift that referenced this pull request Sep 5, 2023
@MaxDesiatov
Merge pull request #67956 from kabiroberai/kabir/ld-path-option
a7ff6e9 
This PR defines an `--ld-path` option for the Swift Driver, which is forwarded to the Clang linker-driver option with the same name. This change is required to support the `toolset.linker.path` option in [SE-0387](https://github.com/apple/swift-evolution/blob/main/proposals/0387-cross-compilation-destinations.md).

## See Also

Downstream: swiftlang/swift-driver#1416, swiftlang/swift-package-manager#6719.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd left review comments

@MaxDesiatov MaxDesiatov MaxDesiatov approved these changes

@artemcm artemcm artemcm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kabiroberai @MaxDesiatov @compnerd @artemcm