-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[Ruby 3.0] Switch back to the original commander gem and update dependencies #18599
Conversation
a02613d
to
1655dda
Compare
fe04881
to
0a7b7c9
Compare
I managed to replicate the same behaviour without forking but with only a few additions using the official way to customise |
98b39d8
to
f04754d
Compare
0c27b66
to
cbf2b61
Compare
cbf2b61
to
d0face1
Compare
381f99d
to
c443c9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty dope!! Let's see if this fixed some bugs with the forked version. Great work, as always🚀
@minuscorp Thank you for your reviews.
As I mentioned, this PR's change doesn't change much in terms of its behaviour🙂 Will work on version bump change soon 🚀 |
@@ -0,0 +1,35 @@ | |||
<%= program :name %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was brought from here.
https://github.com/fastlane/commander/blob/master/lib/commander/help_formatters/terminal_compact/help.erb
I also did some tweaks to substitute these helpers with inline code.
https://github.com/fastlane/commander/blob/master/lib/commander/runner.rb#L200-L223
That's right, maybe a intermediate step would be to just bump the minor version which shouldn't break compatibility to see if that solves the issue, I can try it later on when this gets merged 🚀 |
The thing is that commander bumped highline's version in a patch version change at 4.4.7😅 https://github.com/commander-rb/commander/blob/master/History.rdoc#447--2018-10-22- It doesn't matter as Bundler won't try to update to 4.4.7 but feels like using the equivalent version as before would be safer for now. |
371fa71
to
fff41c2
Compare
@joshdholtz @minuscorp Fixed the issue😄 |
3b56702
to
26082aa
Compare
- run: | ||
name: debug | brew version | ||
command: | | ||
brew -v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we started using orbs for shellcheck, normal testing doesn't need homebrew.
- run: | ||
name: debug | brew version | ||
command: | | ||
brew -v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, fastlane.swift uses swiftformat installed via brew bundle
. This would need this debug info.
26082aa
to
5eccd33
Compare
name: Setup Build | ||
command: | | ||
mkdir -p ~/test-reports | ||
brew update # Needed because this lane uses "brew bundle" and CircleCI's brew install is too old for that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we normally don't need this brew update
...
#17863
I don't know why it failed here though. If the brew version matters, using a newer macOS image might (or might not) help it🤔 Adding brew -v
here would help to see if it's using the right version.
https://app.circleci.com/pipelines/github/fastlane/fastlane/2629/workflows/c48b73e5-02a7-4563-bf8f-8e19cbb6e508/jobs/66023
.circleci/config.yml
Outdated
@@ -270,8 +265,8 @@ workflows: | |||
ruby_version: 'fastlanetools/ci:0.3.0' | |||
- validate_fastlane_swift_generation: | |||
name: 'Validate Fastlane.swift generation' | |||
xcode_version: '11.0.0' | |||
ruby_version: '2.5' | |||
xcode_version: '12.3.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of this reason, I updated used Xcode and Ruby versions here.
https://github.com/fastlane/fastlane/pull/18599/files#r624535522
https://circle-macos-docs.s3.amazonaws.com/image-manifest/v4250/index.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least we're using Xcode 12.4 for latest Swift releases if you want another version bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumped to 12.4 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subcommands now work! This is so 🔥 This looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great to me. Great addition and less deprecated versions to maintain 🚀
Let's archive this 🙂 https://github.com/fastlane/commander |
Hey @ainame 👋 Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉 Please let us know if this change requires an immediate release by adding a comment here 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congratulations! 🎉 This was released as part of fastlane 2.182.0 🚀
💯 ☝️ @joshdholtz I think you're the only one with permission to do so? 😊 |
Checklist
bundle exec rspec
from the root directory to see all new and existing tests passbundle exec rubocop -a
to ensure the code style is validMotivation and Context
As per #17931, I've been doing the migration to get Ruby 3.0 supported in fastlane. I recognised that
highline
gem is a blocker for that as it has invalid usage of Ruby 3.0. We need to update that library version to 2.x (is 1.7.2 right now). To do so, we also need to update another dependencycommander-fastlane
that also depends onhighline
. Due to historical reasons, we switch the use of the originalcommander
to the forked one.It tweaks its styling in the help banner specifically for how to indicate "default command" amongst subcommand options. fastlane/commander#1
For example,
(* default)
and*
asterisk are the ones we have added to the forked one.This is all the fork has. After digging through the gem, I found the official way to customise this help description injecting a custom template to be rendered to
commander
. That way we lose the reason to maintain the forked repo anymore. This PR ported the customisation to fastlane's codebase and stopped using the forked gem.Description
This PR focuses on switching back to the originalcommander
so, in the gemspec file, I locked the gem version to 4.4.4 which is equivalent to the head of the forked one. (When the version was 4.4.3, it was forked and 4.4.3 and was bumped to 4.4.6 with some warning fixes back ported fastlane/commander#6)Next PR will focus oncommander
andhighline
to be updated to get rid of Ruby's deprecation warnings.#18622
It turned out that there isn't really change needed to migrate
commander
andhighline
other than a minor fix for tests. So this PR also bumps the version to the latest version on both gems.https://github.com/commander-rb/commander/blob/master/History.rdoc
JEG2/highline#248 (comment)
Testing Steps
See if
bundle exec fastlane --help
orbundle exec fastlane MODULE --help
looks exactly the same as before.