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

[action][spm] add simulator flag for swift compiler #21707

Merged
merged 17 commits into from Dec 19, 2023

Conversation

gharary
Copy link
Contributor

@gharary gharary commented Dec 9, 2023

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.
  • I've added or updated relevant unit tests.

Motivation and Context

This PR adds extra flag to the spm action, to be able to pass the simulator which swift build is going to compile on that.

Description

Currently, using spm action, will run the swift compiler on the first available simulator. Which is an issue if running on CI server, as some SPM packages only supports iOS platform and running the compiler on a MacOS platform would fail the build. This PR will add an extra flag to the spm action, so the user can pass the simulator on which the swift compiler should run on.
The way it works is that, currently swift compiler doesn't support a simulator flag, therefore we are passing it as a Xswiftc flag. First specifying which SDK the compiler is going to use, by passing the sdk flag. Then using xcrun to locate tools within the Xcode environment which helps find the path to the specified SDK. Then adding the target flag which is used to set the target architecture for the Swift compiler and finally passing the target platform and architecture.
Now user can sets the simulator and/or architercure of the SDK that is going to be used for compiler.

Testing Steps

Adding four unit tests to test the commands

@lacostej lacostej changed the title Add simulator flag to spm action for swift compiler [action][spm] add simulator flag for swift compiler Dec 11, 2023
Copy link
Collaborator

@lacostej lacostej left a comment

Choose a reason for hiding this comment

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

See comment

fastlane/lib/fastlane/actions/spm.rb Outdated Show resolved Hide resolved
Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Congrats on your first PR in the fastlane project @gharary!

I left a question and a couple suggestions. Aside from those, would you mind adding unit tests to this PR? fastlane/spec/actions_specs/spm_spec.rb contains extensive tests already, in case you need sample code to look at when implementing these tests 😊 Let me know if you need any help!

Looking forward to getting this PR merged; the lack of simulator flag is really annoying! Thanks for working on this! 💪

fastlane/lib/fastlane/actions/spm.rb Outdated Show resolved Hide resolved
fastlane/lib/fastlane/actions/spm.rb Outdated Show resolved Hide resolved
fastlane/lib/fastlane/actions/spm.rb Outdated Show resolved Hide resolved
gharary and others added 3 commits December 13, 2023 18:35
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Just a few more changes @gharary 😊 looking good!

fastlane/lib/fastlane/actions/spm.rb Outdated Show resolved Hide resolved
fastlane/lib/fastlane/actions/spm.rb Outdated Show resolved Hide resolved
fastlane/lib/fastlane/actions/spm.rb Outdated Show resolved Hide resolved
fastlane/lib/fastlane/actions/spm.rb Show resolved Hide resolved
fastlane/lib/fastlane/actions/spm.rb Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Since it looks like the macOS simulator command wasn't working (? can't really tell for sure ATM), is there any way we can add a unit test that makes sure not only the output command is the expected string, but that the command actually works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to find a way to actually test the code in unit tests. I was thinking of using open3 to catch the result of executing command, but then testing the swift build command needs an actual SPM. So wasn't making sense to me. Any suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

Uhm, if we wanted, we could create a sample SPM just for testing purposes. But, on a second thought, I think we actually might not need this, as long as we test it manually and make sure it works with all configurations, this should be fine. The reason is that we'd be validating whether "swift build" works, and that's not really the intent of what fastlane's unit tests should be :)

So this is good as is 🙏 thank you!

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Just two minor nitpicks left related to code style, otherwise this PR looks good to go @gbrhaz ! Thanks all the hard work here! 💪 🎉

fastlane/lib/fastlane/actions/spm.rb Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Uhm, if we wanted, we could create a sample SPM just for testing purposes. But, on a second thought, I think we actually might not need this, as long as we test it manually and make sure it works with all configurations, this should be fine. The reason is that we'd be validating whether "swift build" works, and that's not really the intent of what fastlane's unit tests should be :)

So this is good as is 🙏 thank you!

fastlane/lib/fastlane/actions/spm.rb Outdated Show resolved Hide resolved
gharary and others added 2 commits December 18, 2023 19:05
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
@gharary
Copy link
Contributor Author

gharary commented Dec 18, 2023

I did test the shadowing and it works fine and also tests are passing so I think all good.

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

🚀 awesome work! Thank you for the patience and for this contribution @gharary ! 🙏

@rogerluan rogerluan merged commit 287d910 into fastlane:master Dec 19, 2023
1 check passed
SubhrajyotiSen pushed a commit to KeepTruckin/fastlane that referenced this pull request Jan 17, 2024
* Add simulator flag to spm action for swift compiler

* add user entry syntax check

* removing ipadsimulator from syntax check

* Adding verify_block for flag check

* adding simulators list to user_error

* Update fastlane/lib/fastlane/actions/spm.rb

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

* Update fastlane/lib/fastlane/actions/spm.rb

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

* Adding unit tests for simulator

* Fix simulator typo

* Update fastlane/spec/actions_specs/spm_spec.rb

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

* Fix typo.

* Adding support for different simulator architecture

* Adding unit tests for simulator architecture

* Modify code by extracting the logic into methods

* Update fastlane/lib/fastlane/actions/spm.rb

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

* Update fastlane/lib/fastlane/actions/spm.rb

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

---------

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
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.

None yet

3 participants