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

Fixed build failing for iOS and iPad #23

Merged
merged 9 commits into from Jul 5, 2022
Merged

Fixed build failing for iOS and iPad #23

merged 9 commits into from Jul 5, 2022

Conversation

snkirov
Copy link
Contributor

@snkirov snkirov commented Jun 21, 2022

The build was failing for iOS and iPad, due to the package wanting to access the processinfo.username property. This property keeps track of which user is currently logged in on MacOS and thus is unavailable on iOS and iPad, where we only have a single user.

This short PR fixes the problem.

Link to Apple documentation for reference.

No processInfo username property on iPad and iOS
@snkirov snkirov marked this pull request as ready for review June 21, 2022 23:04
Copy link
Owner

@slashmo slashmo left a comment

Choose a reason for hiding this comment

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

Hey @snkirov, thanks for the pull request 🚀 Running on iOS/iPadOS wasn't something I considered when initially building the library since it's a bit more geared towards server-side use. Nonetheless, I want to at least make it compile on these platforms for folks who'd find it useful there too.

@snkirov
Copy link
Contributor Author

snkirov commented Jun 22, 2022

Hey @snkirov, thanks for the pull request 🚀 Running on iOS/iPadOS wasn't something I considered when initially building the library since it's a bit more geared towards server-side use. Nonetheless, I want to at least make it compile on these platforms for folks who'd find it useful there too.

Hey @slashmo. I am currently developing an Observability framework with logs, metrics, and traces in Swift for mobile applications. Your framework has been of great help since I am relying on it for exporting the traces. Even though iOS and iPadOS are not the main focus of this library, it's great that it supports them.

Would you be interested if I were to add build tests for iOS and iPadOS so that similar issues can be avoided in the future?

@snkirov snkirov requested a review from slashmo June 22, 2022 10:40
Copy link
Owner

@slashmo slashmo left a comment

Choose a reason for hiding this comment

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

LGTM 👏

Unfortunately I need to fix something with the SwiftPM caching in GitHub Actions before merging. Would you mind rebasing your PR onto the new main once I'm done with that? 🙏

@slashmo
Copy link
Owner

slashmo commented Jun 22, 2022

Would you be interested if I were to add build tests for iOS and iPadOS so that similar issues can be avoided in the future?

Definitely, great idea! 👍 As mentioned above, I need to rework the CI a bit in general, but if you'd be up for it I'd love to have it run the tests on an iOS/iPadOS simulator as well.

@slashmo slashmo self-assigned this Jun 22, 2022
@snkirov
Copy link
Contributor Author

snkirov commented Jun 28, 2022

Would you mind rebasing your PR onto the new main once I'm done with that? 🙏

No worries at all. Just ping me when you are ready and I will do it.

@snkirov
Copy link
Contributor Author

snkirov commented Jun 29, 2022

Would you be interested if I were to add build tests for iOS and iPadOS so that similar issues can be avoided in the future?

Definitely, great idea! 👍 As mentioned above, I need to rework the CI a bit in general, but if you'd be up for it I'd love to have it run the tests on an iOS/iPadOS simulator as well.

Regarding this I figured out what has to be done, in order for iOS tests to run as well. I am not sure, whether I should commit these changes to this PR or open a new one, so for now I will just describe the steps.

Firstly 2 more schemes need to be added to the project:

  • OtlpGRPCSpanExportingTests
  • OpenTelemetryTests

Then, the following commands have to be executed on the host, so that the tests can be ran.
xcodebuild -scheme OpenTelemetryTests test -sdk iphonesimulator -destination 'platform=iOS Simulator,name=iPhone 13'

xcodebuild -scheme OtlpGRPCSpanExportingTests test -sdk iphonesimulator -destination 'platform=iOS Simulator,name=iPhone 13'

xcodebuild -scheme OpenTelemetryTests test -destination 'platform=iOS Simulator,name=iPad Air (5th-generation)'

xcodebuild -scheme OtlpGRPCSpanExportingTests test -destination 'platform=iOS Simulator,name=iPad Air (5th-generation)'

In order to do this, swift command line tools have to be installed on the host doing the CI. I am not completely sure whether tests for both iPhone and iPad are necessary, since they are both considered a part of the iOS platform.

A simple swift test won't work as it doesn't allow us to specify the platform. Link to source.

* [CI] Use Package.swift contents as cache key

* [CI] Add Swift 5.6 to matrix

* [CI] Use relative path for cache keys

* [CI] Compile onboarding example on runner
@slashmo
Copy link
Owner

slashmo commented Jul 1, 2022

@snkirov Thank you very much for investigating! 🙏 I fixed the CI In general now, and looked into possibly running them on an iOS simulator.

Firstly 2 more schemes need to be added to the project

Since we currently don't have a project checked into Git, one thing that works is referencing the Swift Package inside an Xcode Workspace (thanks @fabianfett for the pointer ✌️), then adding a scheme to that workspace. As for the schemes, I think one called "UnitTests" will suffice, configured to run both test suites.

In terms of how to proceed with this PR, I'd suggest that we do the following:

  1. Rebase this PR onto the latest main so that we can get the Linux CI to pass
  2. Merge the PR
  3. Add the workspace and scheme in a separate PR (I can do that since I already have it locally)
  4. Run the UnitTests scheme on an iPhone simulator as part of the CI GitHub Actions workflow (This could be something you could take on if you're interested)

Does that sound good? Again, thanks for your effort in getting this PR/project forward ❤️

@slashmo slashmo added this to the 0.4.0 milestone Jul 1, 2022
@snkirov
Copy link
Contributor Author

snkirov commented Jul 5, 2022

Does that sound good? Again, thanks for your effort in getting this PR/project forward ❤️

@slashmo This sounds like a solid plan. I rebased and pushed the changes. So now you can merge the PR and open the new one with the workspace & schemes.

Run the UnitTests scheme on an iPhone simulator as part of the CI GitHub Actions workflow (This could be something you could take on if you're interested)

For the final step, do I need to open a new PR in order to edit the github actions, or how does it work? Otherwise, I would love to do it.

@slashmo
Copy link
Owner

slashmo commented Jul 5, 2022

Awesome, thank you! 🙏

For the final step, do I need to open a new PR in order to edit the github actions, or how does it work?

Yep, I think adding a new job (something like unit-test-ios to the CI workflow file would be nice: https://github.com/slashmo/opentelemetry-swift/blob/main/.github/workflows/ci.yaml
That way it would run in parallel with the unit tests on Ubuntu. I'll ping you here once I added the Xcode workspace to the project.

@slashmo slashmo merged commit eb3962a into slashmo:main Jul 5, 2022
@slashmo
Copy link
Owner

slashmo commented Jul 5, 2022

@snkirov I now merged the Xcode workspace (#30) and verified that the following works locally:

xcodebuild -workspace OpenTelemetry.xcworkspace -scheme UnitTests -sdk iphonesimulator -destination 'platform=iOS Simulator,name=iPhone 13 Pro' test

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

Successfully merging this pull request may close these issues.

None yet

2 participants