Re-enable more plugin related tests for non-macOS platforms #8558
Conversation
|
@swift-ci please test |
cmcgee1024
requested review from
bnbarham,
MaxDesiatov,
jakepetroules,
xedin,
dschaefer2,
shawnhyam,
bripeticca,
plemarquand,
owenv,
bkhouri and
daveyc123
as code owners
|
Please ensure that CI passes on Amazon Linux 2 before merging. I've started one here https://ci.swift.org/job/swift-PR-toolchain-amazonlinux2/66/, but you can run your own with cross-repo testing on the toolchain repository with |
|
@swift-ci build toolchain amazon linux 2 |
It doesn't appear that the swift-ci bot is able to test this PR with AL2. I'll monitor this build and see if it passes. |
No, not on this repo. What I'm referring to is a setup with a mock PR on the toolchain repo and cross-repo testing to launch AL2 toolchain build like I did here swiftlang/swift#65101 (comment) |
There was a problem hiding this comment.
This is currently failing on Amazon Linux 2:
Test Case 'BuildCommandSwiftBuildTests.testBuildCompleteMessage' started at 2025-04-25 14:17:07.961
**** FAILURE EXECUTING SUBPROCESS ****
output: Building for debugging...
0%: Pre-planning 1 / 61
0%: Planning deferred tasks
stderr: error: working-directory unsupported on this platform
error: working-directory unsupported on this platform
error: Build failed
<EXPR>:0: error: BuildCommandSwiftBuildTests.testBuildCompleteMessage : threw error "terminated(1): /home/build-user/build/buildbot_linux/swiftpm-linux-x86_64/x86_64-unknown-linux-gnu/release/swift-package-manager --package-path /tmp/DependencyResolution_Internal_Simple.iQ4PNd/DependencyResolution_Internal_Simple --configuration debug --build-system swiftbuild output:
Building for debugging...
0%: Pre-planning 1 / 61
0%: Planning deferred tasks
error: working-directory unsupported on this platform
error: working-directory unsupported on this platform
error: Build failed"
Test Case 'BuildCommandSwiftBuildTests.testBuildCompleteMessage' failed (1.724 seconds)
Test Suite 'BuildCommandSwiftBuildTests' failed at 2025-04-25 14:17:09.686
Executed 1 test, with 1 failure (1 unexpected) in 1.724 (1.724) seconds
|
@swift-ci please test |
I've put another guard on the other test to skip it on AL2. |
|
Kicked off https://ci.swift.org/job/swift-PR-toolchain-amazonlinux2/67/ to verify |
MaxDesiatov
dismissed
their stale review
Request addressed by skipping the test on AL2
| 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 { |
There was a problem hiding this comment.
How hard it would be to move this out to a separate function that could be reused across tests?
There was a problem hiding this comment.
Either way, I'm not implying that would block this PR
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@swift-ci test windows |
There are tests that were formerly failing due to missing support for plugins in the old PIF builder.
Enable these test for better coverage with the new builder.
Fix spelling in one of the APIs
Remove macOS-specific linker flag in the PIF builder and use a setting that is target platform neutral
Update skip comments for tests based on new analysis of the failures
Skip test that's failing on Amazon Linux 2