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.

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

[5.10] Fix forwarding of -ld-path #1454

Merged
merged 3 commits into from
Oct 18, 2023
Merged

[5.10] Fix forwarding of -ld-path #1454

merged 3 commits into from
Oct 18, 2023

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Sep 26, 2023 •
edited
Loading

Cherry-pick of #1447.

The Clang linker driver spells this as --ld-path so we can't forward the argument wholesale anymore, since we spell it -ld-path.

(cherry picked from commit 9a357ee)

@MaxDesiatov MaxDesiatov requested a review from artemcm September 26, 2023 15:25
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@artemcm artemcm requested a review from nkcsgexi September 26, 2023 15:45
@MaxDesiatov
Copy link
Contributor Author

@kabiroberai would you mind having a look at the failing macOS test? Could it be something caused by incorrect conflict resolution during cherry-picking?

@kabiroberai
Copy link
Contributor

@MaxDesiatov I think you need to apply the hunk from SwiftDriverTests.swift, specifically the change from -ld-path to --ld-path when asserting against the Clang invocation (L1698 on main, L1694 in release/5.10.)

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@kabiroberai
Copy link
Contributor

@MaxDesiatov I just checked out your branch and it seems the change from -ld-path to --ld-path in Options.swift is missing; after bringing that in, I was able to get the tests passing with HEAD at b79243c.

@MaxDesiatov
Copy link
Contributor Author

I'm curious though why tests on Linux are passing then. Is the test that covers your change not running on Linux then?

@kabiroberai
Copy link
Contributor

kabiroberai commented Sep 28, 2023 •
edited
Loading

I'm confused by that too — the test should run on Linux — but one thing I noticed is that the Linux CI log doesn't include any tests from SwiftDriverTests.swift. Maybe it's a bug with Linux test discovery?

@kabiroberai
Copy link
Contributor

kabiroberai commented Sep 28, 2023 •
edited
Loading

Update: I checked out the Jenkins branch on Linux (Jammy) and ran swift test --filter SwiftDriverTests.SwiftDriverTests/testLinking and it failed until I made the Options.swift change, same as macOS. The fact that it's passing in the actual CI environment is perplexing. Seems CI uses 5.8 though – could it be a now-fixed discovery bug?

@MaxDesiatov
Copy link
Contributor Author

@artemcm have you seen this issue with Linux CI before? Could it be that those tests are disabled on Linux CI explicitly for some reason?

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

kabiroberai and others added 3 commits October 18, 2023 09:38
@kabiroberai @MaxDesiatov
fix indentation
b2488ae 
# Conflicts:
#	Sources/SwiftDriver/Jobs/WindowsToolchain+LinkerSupport.swift
@kabiroberai @MaxDesiatov
Fix forwarding of -ld-path
a337984 
The Clang linker driver spells this as --ld-path so we can't forward the argument wholesale anymore, since we spell it -ld-path.

(cherry picked from commit 9a357ee)

# Conflicts:
#	Sources/SwiftDriver/Jobs/WindowsToolchain+LinkerSupport.swift
@MaxDesiatov
Update SwiftDriverTests.swift
ccc179c 
Add missing `-` when forwarding as `--ld-path`
@MaxDesiatov MaxDesiatov force-pushed the kabir/fix-ld-path-forwarding-5.10 branch from d093478 to ccc179c Compare October 18, 2023 08:38
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

1 similar comment
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov MaxDesiatov merged commit f16a26a into release/5.10 Oct 18, 2023
@MaxDesiatov MaxDesiatov deleted the kabir/fix-ld-path-forwarding-5.10 branch October 18, 2023 19:36
MaxDesiatov added a commit to swiftlang/swift-package-manager that referenced this pull request Oct 18, 2023
@MaxDesiatov
Swift SDKs: fix toolset.linker.path not passed to -ld-path
13b9264 
This reverts commit 57d0a55 and PR #6939.

Now that swiftlang/swift-driver#1447 and its 5.10 counterpart swiftlang/swift-driver#1454 were merged, we can reapply the fix for Swift SDKs linker metadata not being handled.
@MaxDesiatov MaxDesiatov mentioned this pull request Oct 18, 2023
Merged
MaxDesiatov added a commit to swiftlang/swift-package-manager that referenced this pull request Nov 7, 2023
@MaxDesiatov
Swift SDKs: fix toolset.linker.path not passed to -ld-path (#7021)
1d6565c 
This reverts commit 57d0a55 and PR
#6939.

Now that swiftlang/swift-driver#1447 and its 5.10
counterpart swiftlang/swift-driver#1454 were merged,
we can reapply the fix for Swift SDKs linker metadata not being handled.

Resolves rdar://117049947.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artemcm artemcm artemcm approved these changes

@nkcsgexi nkcsgexi Awaiting requested review from nkcsgexi

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

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