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

Create an esy branch with an esy.json and no vendored directories #111

Open
aantron opened this issue Jul 5, 2021 · 10 comments
Open

Create an esy branch with an esy.json and no vendored directories #111

aantron opened this issue Jul 5, 2021 · 10 comments

Comments

@aantron
Copy link
Owner

aantron commented Jul 5, 2021

This might be a good way to reconcile Dream, Piaf, and the rest of the "ecosystem" for esy users.

The branch can probably be maintained by constantly cherry-picking a single commit that adds esy.json and subtracts src/vendor. This could even be done in CI in the long term, but probably we'd rather get rid of src/vendor by actually upstreaming or releasing the protocol projects, instead.

cc @anmonteiro @EduardoRFS for interest.

@EduardoRFS
Copy link

I wonder if removing the dune file at src/vendor would be enough to use the installed version by esy. It could also be an opam file with an additional rm src/vendor/dune

@aantron
Copy link
Owner Author

aantron commented Jul 5, 2021

Actually, it's probably the opposite. We need that file (atm) because it renames all the libraries so that Dream sees them under different names under the dream package. So we actually wouldn't be able to trivially depend on these packages from esy, because the rest of Dream expects these packages to have dream. names. However, I don't think that's a big deal, since probably only dream.http refers to the vendored code, and we can just include a patch for it in the one commit that will be cherry-picked.

@aantron
Copy link
Owner Author

aantron commented Jul 5, 2021

At first glance, it seems easiest to just delete the whole vendor directory, patch src/http/dune, and add an esy manifest. I don't think an opam manifest will work, at least not when it's interpreted by opam, because we'd need to depend on exact commits of these projects. AFAIK opam can do that only with pin-depends, and that only works for direct "development" installs of the using project (Dream). Does esy handle that differently with an opam manifest? If not, it seems we have to use an esy.json.

@aantron
Copy link
Owner Author

aantron commented Jul 6, 2021

I created and pushed the esy branch to at least serve as a PR target.

About the good first issue label: if opening a PR to esy, set the base of the PR to the branch in the GitHub UI:

Снимок

@dinosaure
Copy link
Contributor

On my side, I mostly aware about the initial diff/reason of the httpaf fork and I really would like to have only one httpaf instead of many and only trust into opam-repository. I'm currently aware about the initial diff but it seems that the "first" httpaf is still maintained (and it had several bug fixes - not sure they are incorporate into anmonteiro/httpaf) and some solutions exist about missed features.

Note that it's not about the usage of esy or opam but about which repository of the eco-system we trust.

Then, such situation burns many energies when, for the MirageOS support, I need to fork my own project (paf) to apply a minor patch due to incompatible interfaces. I'm a bit sad with such situation when it requires a certain amount of maintenability and energy (which I have less and less).

PS: I don't want to bring any trouble and if you don't agree with such view, it's fine but I really think that we should be more convergent in the eyes of "all" communities.

@aantron
Copy link
Owner Author

aantron commented Jul 6, 2021

@dinosaure This issue doesn't make Dream diverge any more on http/af, etc., than it already has — it's only about using separate repos for code that is already vendored into Dream, which is possible on esy.

I fully agree with the intent to eventually resolve the forks, and to get everything released into opam.

To my knowledge so far, as of a few months ago, the "main" http/af lacks:

  • HTTPS support.
  • A runtime that is capable of handling protocol upgrades (I am hazy now, but I don't think it can do WebSocket upgrades out of the box).
  • A shared runtime with that can do ALPN to select an HTTP/2 server instead of http/af.

Given that Dream wants to support all of those things, it seems that it has to use the forks. It already is doing that. This issue just improves some situations for esy users.

cc @anmonteiro @seliopou

@dinosaure
Copy link
Contributor

@dinosaure This issue doesn't make Dream diverge any more on http/af, etc., than it already has — it's only about using separate repos for code that is already vendored into Dream, which is possible on esy.

I agree with the current layout of dream which includes projects with (small?) divergences. However, if you take the perspective of opam-repository, the dream.httpaf is, by essence (and it's why you must include it as a part of dream), different to the "real" httpaf.

If you want to provide something for httpaf and dream according to the current state of opam-repository (as I try to do for paf), you must do a choice about which httpaf you want to use because both diverges (and not only on the implementation). It's why I said:

I need to fork my own project (paf) to apply a minor patch due to incompatible interfaces.

Just to be able to plug it with dream and the httpaf available on opam-repository. Of course, such case is more low-level than an usual Dream user but it's a fork and it has a cost. Then, it seems easy with esy to over-write an existing project by an other but as far as I can tell, it's not the politic of opam-repository (at least, pin-depends exists but it's only for a development purpose). These questions are of course outside of the scope of dream and it's more about different visions from esy and opam but we should consider them where MirageOS mainly use opam.

Then, about HTTP/AF support, the only missing feature is the upgrade (but some PRs exist to implement it, they just need some fuel).

Currently, the HTTPS support (even for CoHTTP) is not strictly handled by the project (or the core) itself but by something outside. In the case of dream, HTTPS is handled by gluten instead of httpaf/h2. And, in the case of MirageOS, it's handled by paf. I would not like to say that httpaf does not handle HTTPS, it can and only a piece into the distribution is missing - but it exists outside with paf or gluten.

Then, about ALPN which is intrinsic with HTTPS, the problem is the same. paf supports ALPN and can switch to h2 if the client support that. And of course, gluten does that too.

So I really think that a solution can exist with some effort (of course) about the connection upgrade. And, on this subject, you can view: inhabitedtype/httpaf#159 and inhabitedtype/httpaf#203

My mainly point is to be sync with opam-repository instead of to use some forks - even if it's working on esy without pain, it requires some plumbing into opam and assert an incompatibility into an eco-system which requires to do a choice between httpaf from @anmonteiro or httpaf from @seliopou. And, from the opam-repository perspective, @seliopou is the owner of httpaf.

@aantron
Copy link
Owner Author

aantron commented Jul 7, 2021

My mainly point is to be sync with opam-repository instead of to use some forks

We already agree on the main point. The question is, can we get all of

  • HTTPS support.
  • A runtime that is capable of handling protocol upgrades (I am hazy now, but I don't think it can do WebSocket upgrades out of the box).
  • A shared runtime with that can do ALPN to select an HTTP/2 server instead of http/af.

If not, then we can't use the opam-repository http/af for now. From your message, it seems that WebSocket upgrades are the last thing missing. Can you confirm?

This isn't the point of this issue, however. Please open a new issue to discuss that. We are already using forks and that's not going to change by creating an esy branch. It will just be a temporary convenience for esy users.

@dinosaure
Copy link
Contributor

If not, then we can't use the opam-repository http/af for now. From your message, it seems that WebSocket upgrades are the last thing missing. Can you confirm?

Yes, I can confirm. On my side with paf, we handle TLS with ocaml-tls and ALPN.

This isn't the point of this issue, however. Please open a new issue to discuss that. We are already using forks and that's not going to change by creating an esy branch. It will just be a temporary convenience for esy users.

Thanks, I will make an issue when I finish my MirageOS support 👍.

@aantron
Copy link
Owner Author

aantron commented Oct 14, 2024

I've since first renamed the vendored libraries, and now replaced them with depending on httpun in #351. I think this issue is likely obsolete.

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

No branches or pull requests

3 participants