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

fixing the cli option retry_download_count to support simulator #405

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

gunesmes
Copy link

@gunesmes gunesmes commented Sep 14, 2020

While I was submitting my PR #404, it was already solved by @freddi-kit with #400. However this PR includes an update for downloading simulators but the corresponding part was not implemented to simulators.rb so this will cause a bug for downloading simulators, as following:

bash-3.2$ xcversion simulators --install='iOS 11.3 Simulator' --retry-download-count=5

simulators --install=iOS 11.3 Simulator --retry-download-count=5
[!] Unknown option: `--retry-download-count=5`
Did you mean: --force?

Usage:

    $ xcversion simulators

      List or install iOS simulators.

Options:

    --install=name   Install simulator beginning with name, e.g. 'iOS 8.4', 'tvOS
                     9.0'.
    --force          Install even if the same version is already installed.
    --no-install     Only download DMG, but do not install it.
    --no-progress    Don’t show download progress.
    --verbose        Show more debugging information
    --no-ansi        Show output without ANSI codes
    --help           Show help banner of specified command
bash-3.2$

or

bash-3.2$ xcversion simulators --install='iOS 11.3 Simulator'

simulators --install=iOS 11.3 Simulator
Installing iOS 11.3 Simulator for Xcode 11.5.0...
Traceback (most recent call last):
	6: from /Users/mesut/.rbenv/versions/2.7.1/bin/xcversion:23:in `<main>'
	5: from /Users/mesut/.rbenv/versions/2.7.1/bin/xcversion:23:in `load'
	4: from /Users/mesut/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/xcode-install-2.6.6/bin/xcversion:12:in `<top (required)>'
	3: from /Users/mesut/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/claide-1.0.3/lib/claide/command.rb:334:in `run'
	2: from /Users/mesut/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/xcode-install-2.6.6/lib/xcode/install/simulators.rb:26:in `run'
	1: from /Users/mesut/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/xcode-install-2.6.6/lib/xcode/install/simulators.rb:45:in `install'
/Users/mesut/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/xcode-install-2.6.6/lib/xcode/install.rb:531:in `install': wrong number of arguments (given 2, expected 3) (ArgumentError)

This PR is fixing this

Simulator

without --retry_download_count, default is 3

bash-3.2$ xcversion simulators --install='iOS 11.0 Simulator'

simulators --install=iOS 11.0 Simulator
  9 1989M    9  189M    0     0  1677k      0  0:20:14  0:01:55  0:18:19 1805k%
curl: (18) transfer closed with 1886829804 bytes remaining to read
  5 1799M    5  105M    0     0  1697k      0  0:18:05  0:01:03  0:17:02 1811k%
curl: (18) transfer closed with 1776103408 bytes remaining to read
  9 1693M    9  163M    0     0  1590k      0  0:18:10  0:01:45  0:16:25 1541k%
curl: (18) transfer closed with 1604346613 bytes remaining to read
%[!] Failed to download iOS 11.0 Simulator.

with --retry-download-count=5 will try to dowload 5 times

bash-3.2$ xcversion simulators --install='iOS 10.3.1 Simulator' --retry=5

simulators --install=iOS 10.3.1 Simulator --retry=5
 13 1198M   13  164M    0     0   717k      0  0:28:30  0:03:54  0:24:36  861k%
curl: (18) transfer closed with 1084795514 bytes remaining to read
 34  865M   34  298M    0     0  1421k      0  0:10:23  0:03:34  0:06:49 1855k%
curl: (18) transfer closed with 595033804 bytes remaining to read
 31  528M   31  163M    0     0  1653k      0  0:05:27  0:01:41  0:03:46 1737k%
curl: (18) transfer closed with 381830355 bytes remaining to read
 45  364M   45  163M    0     0  1194k      0  0:05:12  0:02:20  0:02:52  354k%
curl: (18) transfer closed with 209942487 bytes remaining to read
 53  200M   53  107M    0     0  1510k      0  0:02:15  0:01:12  0:01:03 1760k%
curl: (18) transfer closed with 97659610 bytes remaining to read
%[!] Failed to download iOS 10.3.1 Simulator.
bash-3.2$

@gunesmes
Copy link
Author

@jpsim @freddi-kit could you please review this?

lib/xcode/install/simulators.rb Outdated Show resolved Hide resolved
@freddi-kit
Copy link
Contributor

freddi-kit commented Sep 14, 2020

Thank you for fixing my bug! 👍 I did not notice

@gunesmes
Copy link
Author

@freddi-kit could you please review changes?

@freddi-kit
Copy link
Contributor

@gunesmes
Could you help to run bundle exec rubocop -a again? robocop is reporting some code have to be fixed.

lib/xcode/install.rb Outdated Show resolved Hide resolved
lib/xcode/install.rb Outdated Show resolved Hide resolved
lib/xcode/install.rb Outdated Show resolved Hide resolved
@gunesmes
Copy link
Author

gunesmes commented Sep 15, 2020

@gunesmes
Could you help to run bundle exec rubocop -a again? robocop is reporting some code have to be fixed.

To solve all offenses, we should have made the number_of_try optional param. I have updated to fix this.
dmg_path = download(progress, nil, number_of_try)

Copy link
Contributor

@freddi-kit freddi-kit left a comment

Choose a reason for hiding this comment

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

Great, Thank you!

@gunesmes
Copy link
Author

@jpsim fixed the issues, could you please accept the pr?

@freddi-kit
Copy link
Contributor

@jpsim who can push to gem? it would be helpful the person who have network issue of curl.

@traviswimer
Copy link

Is there something holding up this pull-request? It sounds like the work has been completed.

I tried just cloning the repo and running from this branch, but I can't seem to get it to work. I think it's likely my lack of Ruby knowledge is the problem though. I get an error running bundle install that it needs an older version of bundler, but using the older version tells me it needs the newer version.

If this isn't ready to be merged, could someone offer some guidance on how I can get this branch working as a temporary fix?

@gunesmes
Copy link
Author

gunesmes commented Oct 5, 2020

@traviswimer someone who have write access should approve this PR and then push the gem for the new release. @joshdholtz @KrauseFx @orta or @steipete

@joshdholtz
Copy link
Member

@gunesmes On it! And just for record... I’m the only one out of those 4 mentioned that are associated with this project ATM so no need to mention them in the future 😉 Just trying to save everyone from some extra GitHub notifications 🙃

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.

@gunesmes One small question before approving/merging! And thanks for thIs PR and the ping ❤️

['--retry-download-count', 'Count of retrying download when curl is failed.']].concat(super)
['--number-of-try', 'How many times try to download DMG file if downloading fails. Default is 3.']].concat(super)
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’m confused on what the reasoning for this rename is. This is technically a breaking change, isn’t it? Was there some issue with retry-download-count?

If anything, I would maybe recommend this to be named retry-count. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

I thought that this would be better for grammar.

Copy link
Author

Choose a reason for hiding this comment

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

I changed the parameter to retry-count

Copy link
Member

Choose a reason for hiding this comment

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

Thank you! I will pull down and play with one more time after I’m done taking the doggo for a walk 😊

Thanks again for this PR! I’ll get it release in an hour or so if all is 💯 (which is looks like it should be)

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.

I think the fetching of options need to be renamed too? 🤔 After this we should be good!

lib/xcode/install/simulators.rb Outdated Show resolved Hide resolved
lib/xcode/install/install.rb Outdated Show resolved Hide resolved
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.

One more thing! I think we want to keep the --retry that was getting passed in to curl

The return around curl is a redundancy around the curl retry I believe 😇

lib/xcode/install.rb Show resolved Hide resolved
@gunesmes
Copy link
Author

gunesmes commented Oct 6, 2020

@joshdholtz to fix the Assignment Branch Condition, separated the curl part of the function to new function as curl_file.

@joshdholtz
Copy link
Member

@gunesmes Thank you! Testing this out one more time by downloading some Xcode but code is looking 💯

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.

Downloading seems good! But had an error installing 🤔

lib/xcode/install.rb Show resolved Hide resolved
@joshdholtz
Copy link
Member

@gunesmes What is this PR originally just for? Was this just for really adding the --retry-count to the simulators command? 😇

@gunesmes
Copy link
Author

gunesmes commented Oct 6, 2020

Yes this is fixing the simulator part for adding retry-count.

@joshdholtz
Copy link
Member

@gunesmes Okay, thank you! I’ll dig in to see what’s going on. I might end up redoing some of the argument name changes to make it backwards compatible with the previous version but I will make sure that parameter works 😇 Hopefully we can get this merged and released for you tomorrow!

@freddi-kit
Copy link
Contributor

How is this progressed?

@gunesmes
Copy link
Author

gunesmes commented Dec 6, 2020

@joshdholtz do you have any update for the your testing?

@pronebird
Copy link

pronebird commented May 22, 2021

Can we please bump the default retry count to 10 because there is no way this succeeds in three attempts.

But on the other side, it looks like xcversion does not really understand what's happening in curl. I still see this as a bug, because it's clear that curl knows that there is more data left to download, yet it stops for no apparent reason.

I think the code handling the retry count should be fixed, i.e:

let CURLE_PARTIAL_FILE = 18
if curl.exitCode == CURLE_PARTIAL_FILE, downloadFileSize < expectedFileSize {
  continueDownload()
} else {
  retryCount += 1
  if retryCount > maxRetryCount {
    reportFailure()
  } else {
    continueDownload()
  } 
}

That's the thing I continue witnessing for years now:

xcversion simulators --install="iOS 10.3.1"          

Installing iOS 10.3.1 Simulator for Xcode 12.4.0...
 13 1883M   13  249M    0     0  2882k      0  0:11:09  0:01:28  0:09:41 2562k%
curl: (18) transfer closed with 1713831788 bytes remaining to read
 22 1634M   22  361M    0     0  2475k      0  0:11:16  0:02:29  0:08:47 2399k%
curl: (18) transfer closed with 1334473327 bytes remaining to read
 12 1272M   12  165M    0     0  3026k      0  0:07:10  0:00:55  0:06:15 3755k%^R
 24 1272M   24  305M    0     0  2764k      0  0:07:51  0:01:53  0:05:58 2404k%
curl: (18) transfer closed with 1013687667 bytes remaining to read
%[!] Failed to download iOS 10.3.1 Simulator.

@gunesmes
Copy link
Author

Update retrial count to 10

@gunesmes gunesmes requested a review from joshdholtz May 27, 2021 12:52
@Onno113
Copy link

Onno113 commented Aug 10, 2021

I am not sure what is missing to put the changes here to a release. It looks like it could be missing some kind of approval from @joshdholtz

I am trying to install simulators with xcversion, but it fails every time. I tried already around 10 times. I would like this to be more reliable, because we want to use this for the setup of our CI machines.

xcversion is an amazing tool and it helps us already a lot with installing different kind of Xcode versions, thanks a lot!
:-)

@gunesmes
Copy link
Author

@joshdholtz this is still waiting for your approval, any chance you can review it?

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

Successfully merging this pull request may close these issues.

7 participants