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.

Sign up for GitHub

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

Jump to bottom

[xcodes] fix issue where xcodes action wouldn't accept beta versions of Xcode #21434

Merged
merged 1 commit into from Aug 28, 2023
Merged

[xcodes] fix issue where xcodes action wouldn't accept beta versions of Xcode #21434

merged 1 commit into from Aug 28, 2023

Conversation

apps4everyone
Copy link
Contributor

@apps4everyone apps4everyone commented Aug 1, 2023 •
edited

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.

Motivation and Context

Try to fix #21369 xcodes does not allow "15.0 Beta 5" as version.

@apps4everyone apps4everyone mentioned this pull request Aug 1, 2023
Closed
4 tasks
@apps4everyone
Copy link
Contributor Author

Hey, sorry for asking directly for some help and review about the issue #21369 With xcodes it is currently not possible to test any Beta version of Xcode.

@snatchev @milch @ainame @joshdholtz maybe some PR feedback would be great. thx

@milch
Copy link
Collaborator

milch commented Aug 5, 2023

I see that the problem is that we added the xcodes action after the previous Xcode beta period, so this is probably the first time anyone has tried to use a beta version with it. It seems that at the time the assumption was made that xcodes would accept ruby-style version strings, but it looks like it uses a more complex pattern that probably doesn't make sense to replicate here: https://github.com/XcodesOrg/xcodes/blob/main/Sources/XcodesKit/Version%2BXcode.swift#L22

milch
milch reviewed Aug 5, 2023
View reviewed changes
Copy link
Collaborator

@milch milch left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! 🚀

fastlane/lib/fastlane/helper/xcodes_helper.rb
Comment on lines 19 to 20
def self.requirement(req)
UI.user_error!("Version must be specified") if req.nil? || req.to_s.strip.size == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

The requirement method was only used in this one place: the verify_block in the xcodes action, as far as I can tell. I think if we are to remove it, it would probably make sense to remove the whole thing and have it as an inline block in the action's ruby file, which matches other verify_block methods. This one was probably pulled out because it was a bit longer and using a rescue block, but generally 1-2 line verification methods are inline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is still used in two places, therefore I left as it was for reuse

Screenshot 2023-08-05 at 21 23 04

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I see. So shouldn't xcversion continue to use the old validation?

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 would rather let xcodes or xcversion let decide to throw an error, fastlane should only pre check for empty strings

Copy link
Member

Choose a reason for hiding this comment

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

I think I agree with @apps4everyone, the underlying tools will error out if the user enter an invalid version. I think that checking just for string validity (empty/whitespaces) is what makes more sense here 👌

@apps4everyone apps4everyone changed the title fix: xcodes-not-valid-rubygem-style fix: [Xcode 15.0 Beta] Xcodes not working with non valid rubygem style Aug 11, 2023
@apps4everyone apps4everyone changed the title fix: [Xcode 15.0 Beta] Xcodes not working with non valid rubygem style [xcodes] Xcode xx Beta: Xcodes not working with non valid rubygem style Aug 28, 2023
@apps4everyone
Copy link
Contributor Author

@rogerluan maybe you have 5 mins? What's missing or what I can do to get here more feedback? Thank you

rogerluan
rogerluan approved these changes Aug 28, 2023
View reviewed changes
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.

Good catch @apps4everyone, thanks for fixing this 🙏

I was the author of the xcodes action and I agree this is a better approach 🙌

This PR LGTM

fastlane/lib/fastlane/helper/xcodes_helper.rb
Comment on lines 19 to 20
def self.requirement(req)
UI.user_error!("Version must be specified") if req.nil? || req.to_s.strip.size == 0
Copy link
Member

Choose a reason for hiding this comment

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

I think I agree with @apps4everyone, the underlying tools will error out if the user enter an invalid version. I think that checking just for string validity (empty/whitespaces) is what makes more sense here 👌

@rogerluan rogerluan changed the title [xcodes] Xcode xx Beta: Xcodes not working with non valid rubygem style [xcodes] fix issue where xcodes action wouldn't accept beta versions of Xcode Aug 28, 2023
@rogerluan rogerluan merged commit 65db20a into fastlane:master Aug 28, 2023
8 checks passed
@apps4everyone
Copy link
Contributor Author

Thx for the review @rogerluan, is there any release planned the next days? Would like to test with Xcode 15 before the release next weeks. thx

@rogerluan
Copy link
Member

No planned release for the time being. You can still test it by modifying your Gemfile, e.g.:

gem 'fastlane', git: 'https://github.com/fastlane/fastlane.git', branch: 'master'

And running bundle install to apply the changes.

@apps4everyone
Copy link
Contributor Author

thx, will test with that

@apps4everyone
Copy link
Contributor Author

work's with master 🎉

@joshdholtz joshdholtz mentioned this pull request Sep 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milch milch milch left review comments

@rogerluan rogerluan rogerluan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

xcodes does not allow "15.0 Beta 5" as version
3 participants
@apps4everyone @milch @rogerluan