Skip to content

Re-enable more plugin related tests for non-macOS platforms #8558

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

Merged
merged 12 commits into from
Apr 25, 2025
Merged

Re-enable more plugin related tests for non-macOS platforms #8558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
589471a
Switch to swiftc as the linker driver in SwiftBuildSupport
cmcgee1024 Apr 10, 2025
0fd1b5f
Merge branch 'main' of https://github.com/swiftlang/swift-package-man…
cmcgee1024 Apr 21, 2025
2ac1cf1
Move linker driver setting into PIF Builder for non-CXX targets, and …
cmcgee1024 Apr 22, 2025
dfd403a
Remove platform-specific linker setting from the PIF Builder
cmcgee1024 Apr 22, 2025
3fb0ef9
Enable a plugin test for non-macOS
cmcgee1024 Apr 22, 2025
d9b8942
Add inherited ldflag
cmcgee1024 Apr 22, 2025
a473c9e
Use the new build setting to silence duplicate library warnings in th…
cmcgee1024 Apr 24, 2025
06c629e
Merge branch 'main' of https://github.com/swiftlang/swift-package-man…
cmcgee1024 Apr 24, 2025
c2b5a24
Skip plugin test that fails on AL2
cmcgee1024 Apr 25, 2025
4dac642
Merge branch 'main' of https://github.com/swiftlang/swift-package-man…
cmcgee1024 Apr 25, 2025
381c940
Move AL2 check into swift build test so that there remains native bui…
cmcgee1024 Apr 25, 2025
1d2951a
Skip test build complete message test on Amazon Linux 2
cmcgee1024 Apr 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Sources/SwiftBuildSupport/PackagePIFBuilder.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,9 +651,9 @@ extension PackagePIFBuilder.LinkedPackageBinary {

init?(dependency: ResolvedModule.Dependency, package: ResolvedPackage) {
switch dependency {
case .product(let producutDependency, _):
guard producutDependency.hasSourceTargets else { return nil }
self.init(name: producutDependency.name, packageName: package.name, type: .product)
case .product(let productDependency, _):
guard productDependency.hasSourceTargets else { return nil }
self.init(name: productDependency.name, packageName: package.name, type: .product)

case .module(let moduleDependency, _):
self.init(module: moduleDependency, package: package)
Expand Down
10 changes: 2 additions & 8 deletions Sources/SwiftBuildSupport/PackagePIFProjectBuilder+Modules.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,20 +504,14 @@ extension PackagePIFProjectBuilder {
}

// Additional settings for the linker.
let baselineOTHER_LDFLAGS: [String]
let enableDuplicateLinkageCulling = UserDefaults.standard.bool(
forKey: "IDESwiftPackagesEnableDuplicateLinkageCulling",
defaultValue: true
)
if enableDuplicateLinkageCulling {
baselineOTHER_LDFLAGS = [
"-Wl,-no_warn_duplicate_libraries",
"$(inherited)"
]
} else {
baselineOTHER_LDFLAGS = ["$(inherited)"]
impartedSettings[.LD_WARN_DUPLICATE_LIBRARIES] = "NO"
}
impartedSettings[.OTHER_LDFLAGS] = (sourceModule.isCxx ? ["-lc++"] : []) + baselineOTHER_LDFLAGS
impartedSettings[.OTHER_LDFLAGS] = (sourceModule.isCxx ? ["-lc++"] : []) + ["$(inherited)"]
impartedSettings[.OTHER_LDRFLAGS] = []
log(
.debug,
Expand Down
8 changes: 5 additions & 3 deletions Tests/CommandsTests/BuildCommandTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,10 +959,12 @@ class BuildCommandSwiftBuildTests: BuildCommandTestCases {

override func testBuildCompleteMessage() async throws {
#if os(Linux)
throw XCTSkip("SWBINTTODO: Need to properly set LD_LIBRARY_PATH on linux")
#else
try await super.testBuildCompleteMessage()
if FileManager.default.contents(atPath: "/etc/system-release").map { String(decoding: $0, as: UTF8.self) == "Amazon Linux release 2 (Karoo)\n" } ?? false {
Copy link
Contributor

Choose a reason for hiding this comment

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

How hard it would be to move this out to a separate function that could be reused across tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way, I'm not implying that would block this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

When I make comments on PRs I have been using conventional comments to help make it clear what's blocking (or not) and the nature of the comment. I hope that it adds to the clarity of each my comments.

It wouldn't be difficult to add a common function for the AL2 detection. I hope that this doesn't become very common since we will need to be supporting that platform with the new build system. But, if it does then there should be a common detection code for it. It could probably be a very simple function.

Copy link
Contributor

@MaxDesiatov MaxDesiatov Apr 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IMO approval/request for changes markers on GitHub make it very clear whether something is blocking or not. I find "conventional comments" hard to read as they mostly add clutter and make comments look as if they were posted by some automation script. In addition, punctuation in English is enough to say whether something is a question or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Like most review systems the granularity is wrong. It needs to be on a per-comment basis since one comment could be a question, and another a blocking issue. Conventional comments add very little clutter on the comment, just a couple of words at the beginning, and increase the clarity significantly.

I know that on here I find that there's a general lack of clarity from one comment to another. I often wonder whether a comment is blocking, or just a high-level thought. What do I need to fix to be able to merge, and what is just a non-blocking suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like most review systems the granularity is wrong. It needs to be on a per-comment basis since one comment could be a question, and another a blocking issue.

Yes! Though I've never seen one that supports that, sadly.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do I need to fix to be able to merge, and what is just a non-blocking suggestion?

This one's easy, there's a summary field when a change is requested blocking the merge, which would contain the reasoning for a request.

throw XCTSkip("Skipping Swift Build testing on Amazon Linux because of platform issues.")
}
#endif

try await super.testBuildCompleteMessage()
}

}
10 changes: 8 additions & 2 deletions Tests/CommandsTests/PackageCommandTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3898,15 +3898,21 @@ class PackageCommandSwiftBuildTests: PackageCommandTestCase {
}

override func testCommandPluginBuildingCallbacks() async throws {
throw XCTSkip("SWBINTTODO: Test fails as plugins are not currenty supported")
throw XCTSkip("SWBINTTODO: Test fails because plugin is not producing expected output to stdout.")
}
override func testCommandPluginBuildTestability() async throws {
throw XCTSkip("SWBINTTODO: Test fails as plugins are not currenty supported")
}

#if !os(macOS)
override func testCommandPluginTestingCallbacks() async throws {
throw XCTSkip("SWBINTTODO: Test fails on inability to find libclang on Linux. Also, plugins are not currently supported")
#if os(Linux)
if FileManager.default.contents(atPath: "/etc/system-release").map { String(decoding: $0, as: UTF8.self) == "Amazon Linux release 2 (Karoo)\n" } ?? false {
throw XCTSkip("Skipping Swift Build testing on Amazon Linux because of platform issues.")
}
#endif
try await super.testCommandPluginTestingCallbacks()
}
#endif

}
16 changes: 8 additions & 8 deletions Tests/CommandsTests/TestCommandTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -701,35 +701,35 @@ class TestCommandSwiftBuildTests: TestCommandTestCase {

#if !os(macOS)
override func testSwiftTestXMLOutputVerifySingleTestFailureMessageWithFlagDisabledXCTest() async throws {
throw XCTSkip("Result XML could not be found. The build fails due to an LD_LIBRARY_PATH issue finding swift core libraries. https://github.com/swiftlang/swift-package-manager/issues/8416")
throw XCTSkip("Result XML could not be found. The build fails because of missing test helper generation logic for non-macOS platforms: https://github.com/swiftlang/swift-package-manager/issues/8479")
}

override func testSwiftTestXMLOutputVerifyMultipleTestFailureMessageWithFlagEnabledXCTest() async throws {
throw XCTSkip("Result XML could not be found. The build fails due to an LD_LIBRARY_PATH issue finding swift core libraries. https://github.com/swiftlang/swift-package-manager/issues/8416")
throw XCTSkip("Result XML could not be found. The build fails because of missing test helper generation logic for non-macOS platforms: https://github.com/swiftlang/swift-package-manager/issues/8479")
}

override func testSwiftTestXMLOutputVerifySingleTestFailureMessageWithFlagEnabledXCTest() async throws {
throw XCTSkip("Result XML could not be found. The build fails due to an LD_LIBRARY_PATH issue finding swift core libraries. https://github.com/swiftlang/swift-package-manager/issues/8416")
throw XCTSkip("Result XML could not be found. The build fails because of missing test helper generation logic for non-macOS platforms: https://github.com/swiftlang/swift-package-manager/issues/8479")
}

override func testSwiftTestXMLOutputVerifyMultipleTestFailureMessageWithFlagDisabledXCTest() async throws {
throw XCTSkip("Result XML could not be found. The build fails due to an LD_LIBRARY_PATH issue finding swift core libraries. https://github.com/swiftlang/swift-package-manager/issues/8416")
throw XCTSkip("Result XML could not be found. The build fails because of missing test helper generation logic for non-macOS platforms: https://github.com/swiftlang/swift-package-manager/issues/8479")
}

override func testSwiftTestSkip() async throws {
throw XCTSkip("This fails due to a linker error on Linux. https://github.com/swiftlang/swift-package-manager/issues/8439")
throw XCTSkip("Result XML could not be found. The build fails because of missing test helper generation logic for non-macOS platforms: https://github.com/swiftlang/swift-package-manager/issues/8479")
}

override func testSwiftTestXMLOutputWhenEmpty() async throws {
throw XCTSkip("This fails due to a linker error on Linux. https://github.com/swiftlang/swift-package-manager/issues/8439")
throw XCTSkip("Result XML could not be found. The build fails because of missing test helper generation logic for non-macOS platforms: https://github.com/swiftlang/swift-package-manager/issues/8479")
}

override func testSwiftTestFilter() async throws {
throw XCTSkip("This fails due to an unknown linker error on Linux. https://github.com/swiftlang/swift-package-manager/issues/8439")
throw XCTSkip("Result XML could not be found. The build fails because of missing test helper generation logic for non-macOS platforms: https://github.com/swiftlang/swift-package-manager/issues/8479")
}

override func testSwiftTestParallel() async throws {
throw XCTSkip("This fails due to an unknown linker error on Linux. https://github.com/swiftlang/swift-package-manager/issues/8439")
throw XCTSkip("Result XML could not be found. The build fails because of missing test helper generation logic for non-macOS platforms: https://github.com/swiftlang/swift-package-manager/issues/8479")
}
#endif
}