Skip to content

Clean up iOS Dependency - #3

Merged
mghaznav merged 2 commits into
mainfrom
ali/ios-package-clean-up
Sep 23, 2026
Merged

mghaznav merged 2 commits into
mainfrom
ali/ios-package-clean-up

Conversation

@mghaznav

Copy link
Copy Markdown
Collaborator
  • Removes the dependency on opentelemetry-swift-core
  • Removes the integration tests dependant on opentelemetry-swift-core
  • Matches the minimum versions specified in the firebase-ios-sdk
  • Cleans up the target definition

@mghaznav
mghaznav requested a review from mrober September 23, 2026 17:49
@mghaznav mghaznav self-assigned this Sep 23, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates the Package.swift configuration to include broader platform support and simplifies the target structure. However, the complete removal of the test suite and the test file is problematic as it eliminates automated validation for C++ interoperability. The reviewer suggests retaining a simplified version of the SwiftCompatibilityTests that does not depend on external OpenTelemetry packages, and recommends explicitly defining the publicHeadersPath to ensure consistent header resolution.

I am having trouble creating individual review comments. Click here to see my feedback.

Package.swift (56-75)

high

Completely removing all tests leaves the package with no automated validation. We can retain the SwiftCompatibilityTests target by removing the opentelemetry-swift-core dependency and keeping a simplified test suite. This ensures that C++ interoperability and basic compilation/linking continue to be validated automatically in CI.

    .testTarget(
      name: "SwiftCompatibilityTests",
      dependencies: [
        "FirebaseTelemetryPersistence",
      ],
      path: ".",
      exclude: [
        "CMakeLists.txt",
        "README.md",
        "LICENSE",
        "src",
        "include",
      ],
      sources: ["tests/swift_compatibility_test.swift"],
      swiftSettings: [
        .interoperabilityMode(.Cxx)
      ]
    ),

tests/swift_compatibility_test.swift (1-95)

high

Instead of deleting the compatibility tests entirely, we should keep a simplified version of this file that does not depend on opentelemetry-swift-core. This ensures we still have automated test coverage validating that the C++ types are correctly exposed and usable via Swift's C++ interop.

// Copyright 2026 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import CxxStdlib
import Foundation
import Testing

@testable import FirebaseTelemetryPersistence

/// The native C++ span structure used for cross-language interoperability.
typealias CppSpan = firebase.telemetry.persistence.Span

/// The native C++ span TraceId.
typealias CppTraceId = firebase.telemetry.persistence.TraceId

struct SwiftCompatibilityTests {

  @Test
  func testInit() async throws {
    let span = CppSpan(
      CppTraceId(high: 123, low: 456),
      789,
      101,
      1000,
      2000,
      std.string("testSpan"),
      firebase.telemetry.persistence.AttributesList()
    )

    #expect(span.trace_id().high == 123)
    #expect(span.trace_id().low == 456)
    #expect(span.span_id() == 789)
    #expect(span.parent_span_id() == 101)
    #expect(span.start_time() == 1000)
    #expect(span.end_time() == 2000)
    #expect(span.name() == "testSpan")
  }
}

Package.swift (41)

medium

Explicitly specifying publicHeadersPath: "include" is highly recommended when the target path is customized to the root directory (.). This avoids ambiguity and ensures that Swift Package Manager and Xcode correctly locate the public headers across different toolchain versions.

      sources: ["src"],
      publicHeadersPath: "include"

@mghaznav

Copy link
Copy Markdown
Collaborator Author

Code Review

This pull request updates the Package.swift configuration to include broader platform support and simplifies the target structure. However, the complete removal of the test suite and the test file is problematic as it eliminates automated validation for C++ interoperability. The reviewer suggests retaining a simplified version of the SwiftCompatibilityTests that does not depend on external OpenTelemetry packages, and recommends explicitly defining the publicHeadersPath to ensure consistent header resolution.

I am having trouble creating individual review comments. Click here to see my feedback.

Package.swift (56-75)

high

Completely removing all tests leaves the package with no automated validation. We can retain the SwiftCompatibilityTests target by removing the opentelemetry-swift-core dependency and keeping a simplified test suite. This ensures that C++ interoperability and basic compilation/linking continue to be validated automatically in CI.

    .testTarget(
      name: "SwiftCompatibilityTests",
      dependencies: [
        "FirebaseTelemetryPersistence",
      ],
      path: ".",
      exclude: [
        "CMakeLists.txt",
        "README.md",
        "LICENSE",
        "src",
        "include",
      ],
      sources: ["tests/swift_compatibility_test.swift"],
      swiftSettings: [
        .interoperabilityMode(.Cxx)
      ]
    ),

tests/swift_compatibility_test.swift (1-95)

high

Instead of deleting the compatibility tests entirely, we should keep a simplified version of this file that does not depend on opentelemetry-swift-core. This ensures we still have automated test coverage validating that the C++ types are correctly exposed and usable via Swift's C++ interop.

// Copyright 2026 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import CxxStdlib
import Foundation
import Testing

@testable import FirebaseTelemetryPersistence

/// The native C++ span structure used for cross-language interoperability.
typealias CppSpan = firebase.telemetry.persistence.Span

/// The native C++ span TraceId.
typealias CppTraceId = firebase.telemetry.persistence.TraceId

struct SwiftCompatibilityTests {

  @Test
  func testInit() async throws {
    let span = CppSpan(
      CppTraceId(high: 123, low: 456),
      789,
      101,
      1000,
      2000,
      std.string("testSpan"),
      firebase.telemetry.persistence.AttributesList()
    )

    #expect(span.trace_id().high == 123)
    #expect(span.trace_id().low == 456)
    #expect(span.span_id() == 789)
    #expect(span.parent_span_id() == 101)
    #expect(span.start_time() == 1000)
    #expect(span.end_time() == 2000)
    #expect(span.name() == "testSpan")
  }
}

Package.swift (41)

medium

Explicitly specifying publicHeadersPath: "include" is highly recommended when the target path is customized to the root directory (.). This avoids ambiguity and ensures that Swift Package Manager and Xcode correctly locate the public headers across different toolchain versions.

      sources: ["src"],
      publicHeadersPath: "include"

Swift Interop tests are no longer required as the interop is done through ObjC, so will not be taking this suggestion.

Applied the publicHeadersPath suggestion

@mghaznav
mghaznav merged commit 81db4c4 into main Sep 23, 2026
12 checks passed
@mghaznav
mghaznav deleted the ali/ios-package-clean-up branch September 23, 2026 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants