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

iTMSTransporter support on Windows #13148

Merged
merged 22 commits into from
Aug 30, 2018
Merged

iTMSTransporter support on Windows #13148

merged 22 commits into from
Aug 30, 2018

Conversation

janpio
Copy link
Member

@janpio janpio commented Aug 17, 2018

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've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

iTMSTransporter is available for Windows, but fastlane's implementation doesn't support it yet.

Description

These commits add support for running fastlane pilot upload / upload_to_testflight and fastlane deliver / upload_to_app_store with binaries on Windows.

If iTMSTransporter is not installed, the tests still pass (as they just generate the command, but don't execute it) but on actual execution of upload_to_testflight or upload_to_app_store you get this error message:

...fastlane/fastlane_core/lib/fastlane_core/ui/interface.rb:141:in `user_error!': [!] Could not find transporter at usual locations. Please use environment variable `FASTLANE_ITUNES_TRANSPORTER_PATH` to specify your installation path. (FastlaneCore::Interface::FastlaneError)

Along the way several smaller bugs in the implementation were fixed and tests added (e.g. for special handling of Xcode >= 9).

@janpio janpio changed the title [WIP] itms on windows [WIP] iTMSTransporter support on Windows Aug 17, 2018
@janpio janpio force-pushed the janpio-itms_on_windows branch 2 times, most recently from 7c7f6d0 to 54ee627 Compare August 17, 2018 21:27
@janpio janpio changed the title [WIP] iTMSTransporter support on Windows iTMSTransporter support on Windows Aug 17, 2018
@janpio janpio mentioned this pull request Aug 17, 2018
4 tasks
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

Some small questions and changes

].each do |path|
return path if File.exist?(path)
end
UI.user_error!("Could not find transporter at usual locations. Please use environment variable `FASTLANE_ITUNES_TRANSPORTER_PATH` to specify your installation path.")
end
Copy link
Member

Choose a reason for hiding this comment

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

Can we add an else here that does that return ''?

I can see a future issue right now here we get that early return on line 203 with return '' unless self.mac? || self.windows? because something was changed but then this if statement wasn't change and then this bug is hard to track down 😊

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually how I already had it in my head when updating this to work on Linux as well, so no problem to already do it now.

end
end
rescue => ex
@errors << ex.to_s
end

exit_status = $?.exitstatus
unless exit_status.zero?
if exit_status != 0
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for not keeping the user of .zero? here? (and the other spot below)

Copy link
Member Author

Choose a reason for hiding this comment

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

tbh: I just don't know what exactly .zero does. Is this functionally equivalent?

Copy link
Member

Choose a reason for hiding this comment

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

It is functionally equivalent but its a better way of making sure that we are comparing an number to a number. I would personally keep the .zero? 😊

@@ -187,13 +185,16 @@ def build_download_command(username, password, apple_id, destination = "/tmp", p

def handle_error(password)
# rubocop:disable Style/CaseEquality
unless password === /^[0-9a-zA-Z\.\$\_]*$/
# rubocop:disable Style/YodaCondition
unless /^[0-9a-zA-Z\.\$\_\-]*$/ === password
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for flopping this comparison around? 🤔

Copy link
Member Author

@janpio janpio Aug 21, 2018

Choose a reason for hiding this comment

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

Now it works, before it didn't.
Super strange, seems to have been a bug all along.
@revolter did some experiments on that.

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay. I didn't think there was a difference in what was on left/right side but wasn't sure. So this could be changed back and things would still work the same?

Copy link
Member Author

@janpio janpio Aug 21, 2018

Choose a reason for hiding this comment

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

No. If you change it back, it doesn't work. That's the strange thing here.

rubocop actually wants to write it the other way around, which is probably why this was the other way around and didn't work in the first place - I had to add the rubocop:disable Style/YodaCondition to quiet it.

Please feel free to test around a bit yourself with irb, maybe we both missed the point. Very possible.

UI.error("")
UI.error("Application specific password you provided using")
UI.error("environment variable #{TWO_FACTOR_ENV_VARIABLE}")
UI.error("is invalid, please make sure it's correct.")
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the .? Its fastlane style to not add those 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? I also added the empty line, so it is not as bad any more, but when text continues in the next line, this is just weird, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Its just the way things have been generally printed out in the rest of fastlane so would probably like to keep things consistent if possible 😊

Copy link
Member Author

Choose a reason for hiding this comment

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

ok.

UI.error("Your account has 2 step verification enabled")
UI.error("Please go to https://appleid.apple.com/account/manage")
UI.error("and generate an application specific password for")
UI.error("the iTunes Transporter, which is used to upload builds")
UI.error("the iTunes Transporter, which is used to upload builds.")
Copy link
Member

Choose a reason for hiding this comment

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

Same here with removing the .

UI.error("To set the application specific password on a CI machine using")
UI.error("an environment variable, you can set the")
UI.error("#{TWO_FACTOR_ENV_VARIABLE} variable")
UI.error("#{TWO_FACTOR_ENV_VARIABLE} variable.")
Copy link
Member

Choose a reason for hiding this comment

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

Same here with removing the .

@janpio janpio dismissed joshdholtz’s stale review August 23, 2018 14:43

remaining stuff should not block merging any more

janpio added a commit that referenced this pull request Aug 27, 2018
(part of it is already in PR #13148)
@joshdholtz
Copy link
Member

@janpio Can we get a rebase on this since I merged one of your PRs that conflicts with a change in here? 😇

@janpio
Copy link
Member Author

janpio commented Aug 28, 2018

Conflict could be solved in GitHub web UI fortunately ;) Still passes @joshdholtz

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

Windows support!!! 🙌

@joshdholtz joshdholtz merged commit 2990de4 into master Aug 30, 2018
@joshdholtz joshdholtz deleted the janpio-itms_on_windows branch August 30, 2018 14:18
@janpio
Copy link
Member Author

janpio commented Aug 30, 2018

(One tiny part of it anyway ;))

@fastlane-bot
Copy link

Congratulations! 🎉 This was released as part of fastlane 2.103.0 🚀

janpio added a commit to janpio/fastlane that referenced this pull request Sep 11, 2018
* define path of itms on Windows

* define itms_path for Windows (C:/Program Files (x86)/itms)

* clean up usage of FastlanePty.spawn

* always use shell script on Windows

* remove single quotes around the password

*  fix ipa checksum generation on Windows

* fix (single) quote usage on Windows
also adapt tests

* itms should return directly instead of waiting for key press (on Windows)

* don't run itms tests that depend on xcode on platforms without Xcode

* new tests with FASTLANE_ITUNES_TRANSPORTER_USE_SHELL_SCRIPT

* new test that simulate being on Windows (even when not)

* relax password regex as app specific password contain `-` by default

* improve error output in case of 2 step verification problems

* manual rubocop -a

* fix broken exit_status checks

* fix broken password check + make rubocop happy

* rework tests to work on all OS

* move return '' into else for itms_path

* rubocop

* exit_status == 0 => exit_status.zero?

* remove trailing dot in UI output
@janpio janpio restored the janpio-itms_on_windows branch September 28, 2018 15:47
@fastlane fastlane locked and limited conversation to collaborators Nov 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants