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

GH-42213: [Swift] Use "--warnings-as-errors" only on CI #42214

Merged
merged 2 commits into from
Jun 22, 2024
Merged

Conversation

abandy
Copy link
Contributor

@abandy abandy commented Jun 20, 2024

Rationale for this change

Downstream clients are getting a conflicting options error when building with the "-warnings-as-errors" swift option.

What changes are included in this PR?

This PR disables the "-warnings-as-errors" swift options by default and enables them for CI builds only. The Swift CI script will enable the options in the Package.swift files before running the build.

Copy link

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@abandy abandy changed the title Add CI specific Package files for building with warnings as errors GH-42213: [Swift] Add CI specific Package files for building with warnings as errors Jun 20, 2024
@assignUser
Copy link
Member

Does swift not have a way of passing additional flags to the compiler similar to the CXX_FLAGS envvar in C++? Or is that an issue with dependencies?

@assignUser
Copy link
Member

Also fyi: #42212

@abandy
Copy link
Contributor Author

abandy commented Jun 20, 2024

Does swift not have a way of passing additional flags to the compiler similar to the CXX_FLAGS envvar in C++? Or is that an issue with dependencies?

I did a search but didn't see this as an option. CMake does support Swift (https://github.com/apple/swift-cmake-examples) so maybe we can switch to CMake (in a future PR) the CI build and it might provide a more flexible way to handle this issue.

@kou
Copy link
Member

kou commented Jun 20, 2024

Oh... How about embedding special marks to Package.swift and using it to enable -warnings-as-error in our CI something like the following?

diff --git a/ci/scripts/swift_test.sh b/ci/scripts/swift_test.sh
index b523e3891d..60ea7bde43 100755
--- a/ci/scripts/swift_test.sh
+++ b/ci/scripts/swift_test.sh
@@ -34,7 +34,9 @@ popd
 
 source_dir=${1}/swift/Arrow
 pushd ${source_dir}
+sed -i.bak -e 's,// build: ,,g' Package.swift
 swift test
+mv Package.swift{.bak,}
 popd
 
 source_dir=${1}/swift/ArrowFlight
diff --git a/swift/Arrow/Package.swift b/swift/Arrow/Package.swift
index 6944d7b910..80c7d78e44 100644
--- a/swift/Arrow/Package.swift
+++ b/swift/Arrow/Package.swift
@@ -46,7 +46,7 @@ let package = Package(
             name: "ArrowC",
             path: "Sources/ArrowC",
             swiftSettings: [
-                .unsafeFlags(["-warnings-as-errors"])
+                // build: .unsafeFlags(["-warnings-as-errors"])
             ]
         ),
         .target(
@@ -56,14 +56,14 @@ let package = Package(
                 .product(name: "Atomics", package: "swift-atomics")
             ],
             swiftSettings: [
-                .unsafeFlags(["-warnings-as-errors"])
+                // build: .unsafeFlags(["-warnings-as-errors"])
             ]
         ),
         .testTarget(
             name: "ArrowTests",
             dependencies: ["Arrow", "ArrowC"],
             swiftSettings: [
-                .unsafeFlags(["-warnings-as-errors"])
+                // build: .unsafeFlags(["-warnings-as-errors"])
             ]
         )
     ]

@raulcd
Copy link
Member

raulcd commented Jun 20, 2024

Oh... How about embedding special marks to Package.swift and using it to enable -warnings-as-error in our CI something like the following?

Or using some environment variable with the "extra flags", I've found this snippet example for what I had in mind: https://gist.github.com/Sorix/21e61347f478ae2e83ef4d8a92d933af

@abandy
Copy link
Contributor Author

abandy commented Jun 20, 2024

Oh... How about embedding special marks to Package.swift and using it to enable -warnings-as-error in our CI something like the following?

Or using some environment variable with the "extra flags", I've found this snippet example for what I had in mind: https://gist.github.com/Sorix/21e61347f478ae2e83ef4d8a92d933af

I looked at the snippet above. Cool idea but it seems that it won't work for Xcode based on the last comment:
"Xcode scheme env variables are for app run, not for build time, that's why this doesn't work."

@abandy abandy force-pushed the 42213 branch 2 times, most recently from 856e891 to 30143dc Compare June 20, 2024 19:10
@lukedg97
Copy link

reposting here to make sure it is seen:

@abandy

I can confirm that I fail to build on main with the error: Conflicting options '-warnings-as-errors' and '-suppress-warnings'

and with the only change being checking out with the command gh pr checkout 42214 i am able to build with no errors

For those who may find this conversation later make sure to right-click on the package in xcode and select the option "Update Package" to make sure that Xcode picks up the change.

Originally posted by @lukedg97 in #42213 (comment)

ci/scripts/swift_test.sh Outdated Show resolved Hide resolved
ci/scripts/swift_test.sh Outdated Show resolved Hide resolved
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@github-actions github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review awaiting merge Awaiting merge and removed awaiting review Awaiting review awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jun 20, 2024
@kou kou changed the title GH-42213: [Swift] Add CI specific Package files for building with warnings as errors GH-42213: [Swift] Use "--warnings-as-errors" only on CI Jun 22, 2024
@kou kou merged commit 4a54950 into apache:main Jun 22, 2024
9 of 16 checks passed
@kou kou removed the awaiting merge Awaiting merge label Jun 22, 2024
Copy link

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 4a54950.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 11 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants