-
Notifications
You must be signed in to change notification settings - Fork 348
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
Replace Typhoeus with async-http
#669
Conversation
Thanks for the PR! this has been top of mind recently with the M1 Mac releases. There's an existing PR that attempts to achieve the same result here #639 - I'm not not familiar enough with either of these APIs to pick one, but I'd love to move at least one of these PRs forward. @igor-makarov what do you think? |
First of all, thanks for all the work you've put in, @jasl!
|
Yes, but for now, I add these changes in the branch for not blocking my work.
It works, my branch can natively running on M1 (see the screenshot above).
For now, I can only run tests on my M1 Mac, Homebrew also suffer this,
I've send another PR CocoaPods/CocoaPods#10346 (this PR is ready)
I'll take a look, the PR not ready yet, hopefully I can complete this in this or next week.
I can try, but I don't prefer this idea, because:
Agree, made Cocoapods running on M1 natively is a long-term work, but we can learn from Homebrew:
|
@amorde could you consider this suggest? I left some opinion in #669 (comment) but if it worthy keeping |
@jasl We aren't tied to Concurrent-Ruby - it's an implementation detail of CDNSource. The function it serves in the current implementation is structuring the concurrent HTTP/2 requests so that as many as possible can be performed in parallel. For what I gather, I'm not very familiar with this type of concurrency so I don't know how to structure it correctly. However, the passing of |
@jasl I've managed to fix the partial HTTP body issue without hurting concurrency. I've pushed the change to your branch. I'm repeating the list from above to focus the discussion:
|
Thank you! I'll check tomorrow
Will separate another PR for this before this one ready to review
It works, I've tested in my M1 Mac Mini, I'll do double check
We should ensure all exist targets pass, I'll also run test on my M1 Mac
A PR replace In addition, some minor PRs |
f246bf9
to
1327f3c
Compare
@jasl I've reviewed CocoaPods/CocoaPods#10347. I'll be waiting for the other items for now. |
Thank you, when #670 getting merged, I'll separate a PR including:
Important: if it getting merged which means CocoaPods no longer support macOS 10.14 system Ruby (2.3.7 if my memory correct) |
Ok, so to review (each item ideally should be separate PR to review):
|
no problem |
I think we can bump Ruby to 2.6 which is shipped with 10.15. If we're going to drop support for 10.14 system ruby, we might as well update to 10.15 system Ruby and use the newest version possible. |
I'm hoping this, I'd like to do some contributions to help CocoaPods future-proof! |
Since CocoaPods/CocoaPods#10346 nearly ready I'll back to work on this PR on weekend, |
@jasl could you start with creating the Ruby deprecation PR? It will get the ball rolling and begin getting reviewed. Minimum Ruby version = 2.6. |
async-http
No problem, one more question, can I upgrade bundler in that PR? since Ruby 2.6 bundled bundler 2.0, and 1.8.7 won't compatible with 2.0's Gemfile.lock |
@jasl not sure I understand, what isn't compatible with current Bundler |
This change not affect users, they install CocoaPods via Rubygems. I see CocoaPods/CocoaPods#10324 also upgrade Bundler. Bundler only be used in development and CI, Besides, Bundler 1.17 has deprecation warning on Ruby 3.0
|
@jasl I think you're right. Also, it seems that going to Ruby 2.6 can allow us to finally get rid of the janky
|
Yes, I believe AS 5.x has problem on Ruby 2.7+ |
Cool, so waiting for a PR with:
|
Correct: Bundler bump (CI & Dev only) will do later |
Thanks for all the work! |
029a79e
to
c30e8eb
Compare
long time no follow this, any news? @igor-makarov |
534e239
to
2cfa702
Compare
Just rebased master, squashed all |
Any news here? |
Hi @jasl - long time! I'm currently a bit in a dilemma - on the one hand, this PR contains much work, replacing one library with another. On the other hand - the libraries that are being replaced have already been updated for Apple Silicon - and I can personally confirm that under a correct Ruby installation, Typhoeus works natively. There's a whole separate issue regarding the term "correct Ruby installation" as apparently the System Ruby on Apple Silicon can't work with native extensions because they get build according to RbConfig which is incorrect. |
I believe |
@jasl this isn't what worries me - I have tested @segiddins what do you think? |
If it’s any help we found problems with Typhoeus and it seems largely unmaintained. I haven’t looked closely at this PR for a while but I would say you could make it lower impact/risk if you build an abstract interface for doing the requests and then support both. Such an approach might make migration easier. We can also compare performance too which should be interesting. Bearing in mind that you'll want to put as much of the request handling in an async block to get as much concurrency as possible - happy to provide advice here, but downloading lots of streams concurrently should be advantageous for a tool like this. |
@ioquatix I'm less worried about the performance impact - there are well-defined places where concurrency is utilized and it's easy to see that it works correctly. I'm more worried about the fact that it's a huge change in a core component and the impact of even a small bug can be catastrophic. Having 2 backends will increase the amount of work even more that has been invested right now and increase the test surface even more. The Typhoeus implementation works for the project's needs and it's not a problem to load it on ASi architecture anymore. |
Having |
Is there anything blocking this PR? Is anything else needed other than resolving conflicts? |
It's mostly a matter of our ability to maintain this going forward + push out potential bug fix releases if we find issues after releasing it. There's been more issues with |
I have business traval these days, and will back home next week, I can help to rebase to master branch to fix conflicts |
@amorde can you point me to the new |
We had some internal issues with our CI machines that seem to be related to this https://twitter.com/churowa/status/1455971248555511808 |
I've gone over this PR one more time and there are several issues still related to using
In addition to these gaps, the change is huge and we definitely don't have sufficient tests in place to verify everything. Much time has passed since the M1 Macs first emerged and there exist versions of It is my opinion that because of all the reasons I've listed above, we should not commit to making this change. Big thanks to @jasl for all the hard work done on this, and my sincere apology for not wanting to merge it. 🙏🏻 |
it's OK, the best is FFI can working good on M1. For your opinions about |
Thanks for taking another look @igor-makarov. Do the same points apply to your original PR that uses |
@amorde To be honest it was a draft and I don't think I was close to finishing it 🤷🏻♂️ |
Ok makes sense. I still do wish we didn't depend on ffi as it tends to be a common source of issues but I understand the reason we aren't there yet. |
Should we close those PRs? |
As a Rubyist, I wish But for a CLI tool, less C-ext dependencies, less trouble to end-users |
Yes, I would have also preferred a Ruby-native implementation, but it's very hard to compete with |
async-http has full support for HTTP/1 and HTTP/2 proxies including TLS.
Is this what you are referring to? https://everything.curl.dev/usingcurl/netrc It seems awfully insecure. I don't think we'd want to support this by default. |
@ioquatix I've seen that the library does offer proxy support. However, I've spent most of yesterday trying to allow to users to enable it as a simple As for |
typhoeus
is the only blocking problem that preventing CocoaPods running on M1 nativelyI have read many GH issues of
ffi
andethon
, it looks like the segfault problem is hard to fix, so I decide to rewrite it.I'm using async-http to replace
typhoeus
+concurrent-ruby
Advantage:
typhoeus
+concurrent-ruby
Disadvange:
Once blocking PRs getting merged, I'll cleanup commits and ready for reviewing