Switch to cohttp(lwt) instead of curl #57

tatchi · 2019-02-20T08:01:22Z

Hello,

This is my attempts (and still in progress) to replace curl with cohttp as proposed in #18

Currently, only the makeRequest function to fetch node version has been changed. Furthermore, I still need to figure out how to use it with https(so currently I used httpurls).

Also, I used the `cohttp_lwt_unix' so I guess it would not work on windows. But I assume that was already the case.

Here's a list of todos:

Make it compatible with https
Switch other curl's depending code (download function)

Please don't hesitate to propose any improvements or even to guide me since I'm new to the Ocaml/ReasonML world :)

Schniz · 2019-02-20T08:21:50Z

Hey @tatchi! Welcome 👋

looks very good. I have actually did used cohttp-lwt-unix before I used curl, so I have a way to help here:

I still need to figure out how to use it with https(so currently I used httpurls).

If you add @opam/ssl and @opam/lwt_ssl to the dependencies (and to the require part), cohttp would know how to call https websites. From their docs:

Note that in order to request an HTTPS page like in the above example, you'll need Cohttp to have been compiled with SSL or TLS. For SSL, you'll need to install both ssl and lwt_ssl before installing cohttp. The TLS route will require installing tls before cohttp.

AFAIK, ssl supports TLS as well, but @opam/tls uses libgmp which needs to be installed as an external dependency on MacOS - this is why I replaced it with curl.

Also, I used the `cohttp_lwt_unix' so I guess it would not work on windows. But I assume that was already the case.

I'm not really sure that cohttp_lwt_unix doesn't work on Windows. I think it just uses the Lwt_unix which uses Unix underneath, that has some Windows support. That being said, using curl and tar makes it harder for Windows support in the first place

tatchi · 2019-02-21T06:45:04Z

AFAIK, ssl supports TLS as well, but @opam/tls uses libgmp which needs to be installed as an external dependency on MacOS - this is why I replaced it with curl.

Yes, I already tried to use tls before submitting but I encoutered the problem you mentioned above. I've tested now with ssland it seems to work fine 😃

I'm not really sure that cohttp_lwt_unix doesn't work on Windows. I think it just uses the Lwt_unix which uses Unix underneath, that has some Windows support. That being said, using curl and tar makes it harder for Windows support in the first place

Good news, I will try to test it on windows once I've finished the other remaining points.

Thanks for your support 👌

tatchi · 2019-02-22T13:39:49Z

Hi @Schniz,

Do you have any idea with the build fails? Seems to work locally on my computer.

Schniz · 2019-02-22T16:07:39Z

Hey! I'll take a look. Looks it happens only on Linux (works on Mac), so maybe that's something with the static linking

Schniz · 2019-02-22T17:14:37Z

It seems (on my machine, running docker build .) that if I use @opam/tls things work. for some reason, static build for openssl doesn't work here. Maybe someone from the Reason Native discord could help out. I'll fire a message in a couple of hours 😄

A weird option is to have MacOS use ssl{,_lwt} and Linux use tls. We test the independently so it would be okay, but package.json wouldn't reflect one of them (we already have a script to make the build static on Linux though)

Schniz · 2019-02-22T20:16:31Z

By the way, I tried adding openssl-dev to the docker build, but it failed to compile anyway. Maybe adding overrides to pesy’s core could be an interesting idea. Or! Having a library that will resolve to the right one based on the platform

tatchi · 2019-02-23T08:23:49Z

Hi,

Thanks for your help 😊 I added libressl-dev to the Dockerfile and now the docker build . command seems to work on my machine.

I pushed this change to check the CI but don't know why it's stuck on the Expected — Waiting for status to be reported.

Schniz · 2019-02-23T17:59:54Z

I have just ran it manually: https://dev.azure.com/galstar0385/fnm/_build/results?buildId=209

tatchi · 2019-02-24T07:06:39Z

Hi @Schniz,

Thanks for the manual build. Unfortunately, this one has failed as well 😞

However (and if I understand correctly), I'm not sure the build has been executed with ma latest commit that changed the dockerfile. Indeed, when I click on the commit link on the build result page, I don't see the changes I've made in this file.

Also, I checked the log and again don't see my latest changes:

2019-02-23T17:59:52.2728181Z Step 2/14 : RUN apk add --no-cache nodejs bash npm curl g++ make m4 patch gmp-dev perl git jq

In my latest commit I've added the libressl-dev package at the end of this line.

Did I make something wrong ?

Schniz · 2019-02-24T07:30:26Z

weh

maybe try to push an empty commit? 🤷‍♂️
CI is being funny

Schniz · 2019-02-24T07:30:48Z

By the way, thanks for the efforts 🏆 I hope we can make it pass eventually

Schniz · 2019-02-24T07:31:56Z

You can remove the "replace curl" todo list instead of making an empty commit (we'll need to remove it anyway when this is merged 😄)

tatchi · 2019-02-24T07:49:30Z

You can remove the "replace curl" todo list instead of making an empty commit (we'll need to remove it anyway when this is merged 😄)

Dit it but apparently it has not triggered a build. I also set my PR as ready to review just to check that it was because this was a draft PR.

Could it be due to permission issue? See the comment at the end of this thread:

https://developercommunity.visualstudio.com/content/problem/174787/github-pull-requests-not-triggering-build.html

Schniz · 2019-02-24T08:01:20Z

weeeeeeeird
why merging from master worked? 🤔

Schniz · 2019-02-24T08:01:31Z

conflicts?

tatchi · 2019-02-24T08:07:56Z

conflicts?

Yes, when I switched my PR as ready to review, I saw that there was a conflict. I merged from master, resolved the conflict and pushed. Now the build as triggered.

Sorry for my ignorance 😬

Schniz · 2019-02-24T08:09:08Z

I don't believe it's ignorance - I haven't seen the "there are conflicts" panel on the bottom of the page too. Maybe that's something we can report to GitHub?

Schniz · 2019-02-24T08:14:43Z

There's a failing test, which I think may relate to redirects:

Running test in ./feature_tests/node_mirror_installation/run.sh
---------------------------------------------------------------
fnm: internal error, uncaught exception:
     (Failure hd)
     Raised at file "pervasives.ml", line 32, characters 17-33
     Called from file "list.ml" (inlined), line 27, characters 10-23
     Called from file "library/Compression.re", line 10, characters 17-31
     Called from file "src/core/lwt.ml", line 1929, characters 23-26
     Re-raised at file "library/Compression.re", line 9, characters 2-193
     Re-raised at file "executable/Install.re", line 79, characters 2-104
     Re-raised at file "src/core/lwt.ml", line 2987, characters 20-29
     Called from file "src/unix/lwt_main.ml", line 26, characters 8-18
     Called from file "cmdliner_term.ml", line 25, characters 19-24
     Called from file "cmdliner.ml", line 116, characters 32-39

The Chinese Node.js mirror stores stuff differently but returns a redirection to match https://nodejs.org/dist structure, so we might need to follow redirects on every get request

Schniz · 2019-02-24T08:19:07Z

On the flipside - compilation worked!

tatchi · 2019-02-24T08:25:19Z

I don't believe it's ignorance - I haven't seen the "there are conflicts" panel on the bottom of the page too. Maybe that's something we can report to GitHub?

Indeed, that's was not indicated when the PR was in draft status. But that's probably intended (I don't know).

There's a failing test, which I think may relate to redirects:

Running test in ./feature_tests/node_mirror_installation/run.sh
---------------------------------------------------------------
fnm: internal error, uncaught exception:
     (Failure hd)
     Raised at file "pervasives.ml", line 32, characters 17-33
     Called from file "list.ml" (inlined), line 27, characters 10-23
     Called from file "library/Compression.re", line 10, characters 17-31
     Called from file "src/core/lwt.ml", line 1929, characters 23-26
     Re-raised at file "library/Compression.re", line 9, characters 2-193
     Re-raised at file "executable/Install.re", line 79, characters 2-104
     Re-raised at file "src/core/lwt.ml", line 2987, characters 20-29
     Called from file "src/unix/lwt_main.ml", line 26, characters 8-18
     Called from file "cmdliner_term.ml", line 25, characters 19-24
     Called from file "cmdliner.ml", line 116, characters 32-39

The Chinese Node.js mirror stores stuff differently but returns a redirection to match https://nodejs.org/dist structure, so we might need to redirect automatically

Thx for the help, I will look at that but probably not today as I won't have time.

Anyway, I'm happy that the compilation succeded 😄

Schniz · 2019-02-24T08:56:43Z

Anyway, I'm happy that the compilation succeded 😄

the little things that make us happy!

thanks again for all the hard (and amazing) work!

Schniz · 2019-02-25T10:00:32Z

woohoooooooooo it worksssss!
can you just resolve conflicts and we merge it? 👏 💃 👏

sorry for making the conflicts - but I was sad that #64 failed although it didn't change any source code, and didn't want to leave it as it is

tatchi · 2019-02-25T10:32:05Z

woohoooooooooo it worksssss!
can you just resolve conflicts and we merge it? 👏 💃 👏

sorry for making the conflicts - but I was sad that #64 failed although it didn't change any source code, and didn't want to leave it as it is

I guess it's ok 😃

Schniz · 2019-02-25T10:33:23Z

👏 👏 👏 🏆 🏆 🌮

Again, thank you so much for that!

tatchi added 3 commits February 20, 2019 08:26

added cohttp, lwt dependencies

6d8b89e

switched to lwt to fetch remote versions

28f62a6

https -> http

db58b85

tatchi changed the title ~~Switch to cohttp(lwt) instead of curl~~ [WIP]: Switch to cohttp(lwt) instead of curl Feb 20, 2019

tatchi added 2 commits February 21, 2019 07:33

Added ssl and lwt_ssl dependencies

9a1ad87

switched back to https

bd17afa

tatchi added 3 commits February 21, 2019 10:21

switch from curl to lwt for the download fn

3cfdeb0

removed unused functions

5429405

removed lwt.unix which doesn't seem to be needed

d3bc658

Added libressl-dev to dockerfile

20787c5

removed replace curl from todo list

52bf365

tatchi marked this pull request as ready for review February 24, 2019 07:43

merge from master

8811776

Should follow redirection

94879b0

Schniz added the PR: New Feature A new feature was added label Feb 25, 2019

merge from master

60f079c

tatchi changed the title ~~[WIP]: Switch to cohttp(lwt) instead of curl~~ Switch to cohttp(lwt) instead of curl Feb 25, 2019

Schniz merged commit 3beebc8 into Schniz:master Feb 25, 2019

Schniz mentioned this pull request Feb 25, 2019

Replace curl #18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to cohttp(lwt) instead of curl #57

Switch to cohttp(lwt) instead of curl #57

tatchi commented Feb 20, 2019 •

edited

Schniz commented Feb 20, 2019

tatchi commented Feb 21, 2019

tatchi commented Feb 22, 2019

Schniz commented Feb 22, 2019

Schniz commented Feb 22, 2019

Schniz commented Feb 22, 2019 •

edited

tatchi commented Feb 23, 2019 •

edited

Schniz commented Feb 23, 2019

tatchi commented Feb 24, 2019

Schniz commented Feb 24, 2019

Schniz commented Feb 24, 2019

Schniz commented Feb 24, 2019

tatchi commented Feb 24, 2019

Schniz commented Feb 24, 2019

Schniz commented Feb 24, 2019

tatchi commented Feb 24, 2019

Schniz commented Feb 24, 2019

Schniz commented Feb 24, 2019 •

edited

Schniz commented Feb 24, 2019

tatchi commented Feb 24, 2019

Schniz commented Feb 24, 2019

Schniz commented Feb 25, 2019 •

edited

tatchi commented Feb 25, 2019

Schniz commented Feb 25, 2019

Switch to cohttp(lwt) instead of curl #57

Switch to cohttp(lwt) instead of curl #57

Conversation

tatchi commented Feb 20, 2019 • edited

Schniz commented Feb 20, 2019

tatchi commented Feb 21, 2019

tatchi commented Feb 22, 2019

Schniz commented Feb 22, 2019

Schniz commented Feb 22, 2019

Schniz commented Feb 22, 2019 • edited

tatchi commented Feb 23, 2019 • edited

Schniz commented Feb 23, 2019

tatchi commented Feb 24, 2019

Schniz commented Feb 24, 2019

Schniz commented Feb 24, 2019

Schniz commented Feb 24, 2019

tatchi commented Feb 24, 2019

Schniz commented Feb 24, 2019

Schniz commented Feb 24, 2019

tatchi commented Feb 24, 2019

Schniz commented Feb 24, 2019

Schniz commented Feb 24, 2019 • edited

Schniz commented Feb 24, 2019

tatchi commented Feb 24, 2019

Schniz commented Feb 24, 2019

Schniz commented Feb 25, 2019 • edited

tatchi commented Feb 25, 2019

Schniz commented Feb 25, 2019

tatchi commented Feb 20, 2019 •

edited

Schniz commented Feb 22, 2019 •

edited

tatchi commented Feb 23, 2019 •

edited

Schniz commented Feb 24, 2019 •

edited

Schniz commented Feb 25, 2019 •

edited