com-lihaoyi / requests-scala

A Scala port of the popular Python Requests HTTP client: flexible, intuitive, and straightforward to use.
Other
705 stars 80 forks source link

Scala-Native Support (500USD Bounty) #156

Open lihaoyi opened 2 months ago

lihaoyi commented 2 months ago

STTP Support Scala-Native via Curl. Requests-Scala should be able to support it via the same approach. We just need it to be wired into the Requests-Scala codebase, possibly refactoring out the actual making of the request into src-jvm/ and src-native/ platform-specific code.

Any platform-specific functionality that doesn't work on Scala Native should be called out, explained clearly, and their associated tests moved into platform-specific folders as well

To incentivize contribution, I'm putting a 500USD bounty on resolving this ticket. This is payable via bank transfer, and at my discretion in case of ambiguity.

domaspoliakas commented 2 months ago

I'll give this one a go 🙂

domaspoliakas commented 2 months ago

Just wanted to mention that I am still working on this and have not abandoned it! I have PRed HttpCookie to scala native now https://github.com/scala-native/scala-native/pull/3927 and I'll be looking at how to solve the issue of SSLContext next.

domaspoliakas commented 1 month ago

A wee little update - the approach that sttp has taken unfortunately does not seem viable here; sttp simply does not allow any streaming with the curl backend which is how they got away with only using the libcurl easy API, but given that this lib is based on InputStream at its core - my understanding is that this is not quite possible. That is - curl's easy API does not allow piecemeal consumption of the data, and therefore we'd have to buffer the whole response into memory if we were to use the same approach. However curl's multi API I think has all the pieces that are needed here. There's http4s-curl as prior art which does exactly that, so I'm trying to see if I can adapt a similar approach for this repo.

lihaoyi commented 1 month ago

@domaspoliakas sounds good!

lqhuang commented 1 month ago

I have subscribed to this issue for a long time due to I also want to give a try in the beginning. But I'm a little worry about my junior skills in Scala Naive. Excited that @domaspoliakas is taking over the ticket.

I still hope I could give a hand, or we could cooperate to solve some problems?

Like above mentioned, we're currently trying to build Scala native API on top of libcurl just like https-curl did. I'm thinking could we extract the core libcurl as an individual binding lib, so both https-curl and requests are able to create their user interface on it?

Someone has tried to create a binding (https://github.com/fsat/scala-native-libcurl-bindings) years ago and obviously abandoned already.

What's your opinion?

Regards

domaspoliakas commented 1 month ago

I personally do like the idea (and indeed you could add sttp to the list of libs that could benefit from such a thing here), but one downside is that it would add a dependency. This has a couple of drawbacks. The first is that requests-scala is a very minimalist library and practically does not add any transitive dependencies, which this would change a little. The second is that it would be a dependency outside of this organisation's control. Whether these are blockers or not is for @lihaoyi to decide.

If you do get the green light - take a look at https://github.com/domaspoliakas/requests-scala/blob/scala-native-cross-build/requests/src-native/requests/curl.scala . I've already covered a chunk of the API, at least the things that I needed so far, which you could use as a starting point. I used sttp as a reference for that, so you'll notice that the style of the bindings is in many ways reminiscent of their implementaiton. Another thing to mention is that I of course treated this as an internal detail of requests-scala so it may need some cleanup in order to be published as a library of its own.

lihaoyi commented 1 month ago

I dont mind a third party dep as along as it is well maintained and stable. Most important thing is that the end to end workflow works; we can refactor the implementation however we like later

sideeffffect commented 1 month ago

You could potentially use curl bindings automatically generated by sn-bindgen https://github.com/indoorvivants/sn-bindgen-examples by @keynmol

keynmol commented 1 month ago

Note that sn-bindgen produces Scala3-only bindings. My recommendation for this would be for libraries to ship their own handcrafted bindings as long as the API surface is small.

Alternatively, lots of binding code can be copypasted from https://github.com/indoorvivants/sn-bindgen-examples/blob/main/example-curl/src/main/scala/generated/curl.scala#L3113 and adapted to fit Scala 2. It should be a lot easier than handcrafting the entire API

domaspoliakas commented 1 month ago

The scala-3 requirement was the main reason for hand-crafting the bindings in this case, although I have used the generated bindings as a guide for some of the trickier parts of the API. The API is actually fairly small in the end, and I don't think I'll be adding anything to the bindings at this point.

I'll take this opportunity to mention that parts of the test suite work already, so there is light at the end of the tunnel. There's still a bit of work before all of it is ok (the looming threat is still SSLContext and however I'll need to solve that), and then I'll need to clean it all up, but the end is definitely getting nearer :)

lihaoyi commented 1 month ago

Sounds great @domaspoliakas !

lihaoyi commented 4 days ago

@domaspoliakas any updates here? If not I'll be putting it up in the next set of bounties

domaspoliakas commented 4 days ago

June was unfortunately a tough month to find time for this and I apologise for disappearing as I have. I have been working on this again as of last weekend however. Simple functionality is working at the moment, but there are still quite a few things left:

That last one has eaten quite a lot of my time so far with unfortunately not much to show for it. Generally uploads seem to work (some hand testing and some of the test suite corroborate this), but so far I am yet to find the cause or the right combination of flags to make the multipart test pass.

I am happy to continue working on it now that I have time again, but if you would rather I passed the torch on to someone else - that is totally understandable.


EDIT: got the multipart bug fixed now, so there's that

lihaoyi commented 4 days ago

Got it! If you're still involved then go ahead and keep working on it, just wanted to make sure it isn't dropped