Skip to content

[ECO-5615] Fix error about conflicting warnings-related build options#440

Merged
lawrence-forooghian merged 1 commit intomainfrom
436-warnings-setting-conflict
Oct 21, 2025
Merged

[ECO-5615] Fix error about conflicting warnings-related build options#440
lawrence-forooghian merged 1 commit intomainfrom
436-warnings-setting-conflict

Conversation

@lawrence-forooghian
Copy link
Copy Markdown
Collaborator

@lawrence-forooghian lawrence-forooghian commented Oct 21, 2025

This reverts the configuration changes from a78e4ae. It turns out that this is causing a compilation error of error: conflicting options '-warnings-as-errors' and '-suppress-warnings' when trying to use Xcode to build an app that uses the SDK.

Will create a minimal reproduction and raise a bug with Apple (have created #441 for this and restoring a78e4ae once fixed).

I'm not sure why our example app wasn't catching this, but I guess there must be a difference in the way that the SDK gets built when it's included in the SDK via a workspace as opposed to via SPM. I've created #442 for for making the example app build more realistic to try and catch this sort of issue in the future; tried doing it as part of this work but didn't find a good solution, and want to get this fix released.

Resolves #436.

Summary by CodeRabbit

  • Chores
    • Updated build and test configurations to enforce stricter warning handling during continuous integration.
    • Refactored warning-as-errors settings for improved consistency across build processes.
    • Added documentation on warning handling decisions and related historical context.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Oct 21, 2025

Walkthrough

The PR resolves compiler flag conflicts by removing the global warning-as-errors setting from Package.swift and applying it selectively in CI workflows only. Documentation explaining the rationale is added to XcodeRunner.swift.

Changes

Cohort / File(s) Summary
CI workflow updates
.github/workflows/check.yaml
Replaces swift build invocations with swift build -Xswiftc -warnings-as-errors in build-and-test-spm and build-release-configuration-spm steps. Adds preceding comments with explanatory URLs. Updates corresponding commented test line from #- run: swift test to #- run: swift test -Xswiftc -warnings-as-errors.
Package configuration
Package.swift
Removes the global .treatAllWarnings(as: .error) setting from commonSwiftSettings.
Build tooling documentation
Sources/BuildTool/XcodeRunner.swift
Adds a multi-line comment documenting past issues and design decisions around SWIFT_TREAT_WARNINGS_AS_ERRORS, SWIFT_SUPPRESS_WARNINGS, and related Xcode/Package.swift behavior. No functional changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

The changes follow a consistent, repetitive pattern across CI configuration and are primarily configuration adjustments with one setting removal and added documentation. The logic impact is minimal and well-scoped.

Poem

🐰 Conflicts resolved with careful care,
Flags now placed with methodical flair,
Global settings stepped aside,
CI's strictness, our guide,
Warnings tamed, dependencies spared!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "ECO-5615 Fix error about conflicting warnings-related build options" is directly related to the main change in the changeset. The PR addresses a compilation error triggered by conflicting warnings-related build options, which is exactly what the title describes. The changes remove the global .treatAllWarnings(as: .error) setting from Package.swift, update the workflow to use build-level -Xswiftc -warnings-as-errors flags, and add explanatory documentation. The title is concise, specific, and clearly communicates the primary objective without unnecessary noise.
Linked Issues Check ✅ Passed The linked issues (#436 and ECO-5615) describe a compilation error where users encounter "Conflicting options '-warnings-as-errors' and '-suppress-warnings'" when using the SDK alongside other dependencies. The PR directly addresses this by removing the .treatAllWarnings(as: .error) setting from Package.swift that was causing the conflict, and instead moves the warnings-as-errors enforcement to the build command level via -Xswiftc -warnings-as-errors flags in the workflow. This approach resolves the reported issue while maintaining the ability to treat warnings as errors during the SDK's own build process. The changes align precisely with the requirements described in both linked issues.
Out of Scope Changes Check ✅ Passed All changes in the PR are directly related to fixing the conflicting warnings-related build options error. The removal of .treatAllWarnings(as: .error) from Package.swift directly addresses the root cause, the workflow updates implement the alternative solution, and the multi-line comment added to XcodeRunner.swift serves to document the reasoning and past decisions around this issue. While the comment is primarily explanatory rather than functional, it remains within scope as it provides context and rationale for the configuration changes made to resolve the reported problem.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 436-warnings-setting-conflict

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c96988e and 322a267.

📒 Files selected for processing (3)
  • .github/workflows/check.yaml (2 hunks)
  • Package.swift (0 hunks)
  • Sources/BuildTool/XcodeRunner.swift (1 hunks)
💤 Files with no reviewable changes (1)
  • Package.swift
🧰 Additional context used
📓 Path-based instructions (1)
**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.swift: Use protocol-based design; expose SDK functionality via protocols and prefer associated types with opaque return types (some Protocol) instead of existentials (any Protocol)
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API must use typed throws with ErrorInfo; use InternalError internally and convert at the public API boundary
For public structs emitted by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
Task, CheckedContinuation, and AsyncThrowingStream do not support typed errors; use Result and call .get() to surface typed errors
Do not use Dictionary.mapValues for typed throws; use ablyChat_mapValuesWithTypedThrow instead
When the compiler struggles with typed throws, explicitly declare the error type on do blocks (e.g., do throws(InternalError))
Specify error types in closures using the throws(ErrorType) syntax (e.g., try items.map { jsonValue throws(InternalError) in ... })
Mark any test-only APIs with testsOnly_ prefix and wrap them in #if DEBUG

Files:

  • Sources/BuildTool/XcodeRunner.swift
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: Xcode, iOS (Xcode 26.0)
  • GitHub Check: Xcode, release configuration, iOS (Xcode 26.0)
  • GitHub Check: Xcode, tvOS (Xcode 26.0)
  • GitHub Check: Example app, tvOS (Xcode 26.0)
  • GitHub Check: Xcode, macOS (Xcode 26.0)
  • GitHub Check: Xcode, release configuration, macOS (Xcode 26.0)
🔇 Additional comments (3)
.github/workflows/check.yaml (2)

94-97: Good alignment with the fix strategy.

The changes correctly apply -Xswiftc -warnings-as-errors at the swift build invocation level for SPM builds, addressing the package-level conflict. The consistent update to the commented test line is helpful for future re-enablement.


115-116: Consistent warning-as-errors enforcement across configurations.

The -Xswiftc -warnings-as-errors flag is properly applied to both standard and release configuration SPM builds, ensuring consistent warning treatment.

Sources/BuildTool/XcodeRunner.swift (1)

27-65: Excellent documentation of a complex architectural constraint.

This comment provides crucial context for why the warnings-as-errors setting must be applied selectively in CI rather than globally in Package.swift. The explanation thoroughly covers the problem (compiler flag conflicts with dependencies), failed workarounds, and the accepted trade-off. Future maintainers will appreciate why attempts to fix this at the manifest level will fail, preventing wasted effort on similar approaches.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

This reverts the configuration changes from a78e4ae. It turns out that
this is causing a compilation error of "error: conflicting options
'-warnings-as-errors' and '-suppress-warnings'" when trying to use Xcode
to build an app that uses the SDK.

Will create a minimal reproduction and raise a bug with Apple (have
created #441 for this and restoring a78e4ae once fixed).

I'm not sure why our example app wasn't catching this, but I guess there
must be a difference in the way that the SDK gets built when it's
included in the SDK via a workspace as opposed to via SPM. I've created
#442 for for making the example app build more realistic to try and
catch this sort of issue in the future; tried doing it as part of this
work but didn't find a good solution, and want to get this fix released.

Resolves #436.
@lawrence-forooghian lawrence-forooghian force-pushed the 436-warnings-setting-conflict branch from 881dd22 to 322a267 Compare October 21, 2025 14:09
@lawrence-forooghian lawrence-forooghian changed the title [WIP, ECO-5615] Fix error about conflicting warnings-related build options [ECO-5615] Fix error about conflicting warnings-related build options Oct 21, 2025
@lawrence-forooghian lawrence-forooghian marked this pull request as ready for review October 21, 2025 14:11
Copy link
Copy Markdown
Collaborator

@umair-ably umair-ably left a comment

Choose a reason for hiding this comment

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

good find!

@lawrence-forooghian lawrence-forooghian merged commit 2df015d into main Oct 21, 2025
17 checks passed
@lawrence-forooghian lawrence-forooghian deleted the 436-warnings-setting-conflict branch October 21, 2025 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Getting : Conflicting options '-warnings-as-errors' and '-suppress-warnings'

2 participants