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

Dream — tidy, feature-complete Web framework #18536

Merged
merged 1 commit into from
Apr 22, 2021
Merged

Conversation

aantron
Copy link
Contributor

@aantron aantron commented Apr 20, 2021

Dream is a Web framework with a simple programming model. It features...

Dream is presented as a single, flat module. It does not assume any boilerplate. Indeed, depending on the use case, a Dream app can be packaged as a single executable.

See

Included, in particular, are full-stack (server and client in ML) examples with...

@aantron
Copy link
Contributor Author

aantron commented Apr 20, 2021

CI failures are on older distributions:

(failed: ssl_stubs.c: 'TLS1_3_VERSION' undeclared (first use in this function); did you mean 'TLS1_2_VERSION'?)

I will look into this for future releases, but for alpha1, from Dream's point of view, it's fine not to support building on older distributions.

]

install: [
["opam-installer" "--prefix" prefix "dream.install"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
["opam-installer" "--prefix" prefix "dream.install"]

This should be unnecessary as opam already installs <pkg>.install by default (on top of the install section)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I just tested removing this line with esy, and esy does not install dream.install by default, causing a dependency error later if the line is removed. This may be a bug/oversight, but it's non-obvious and I think it's better not to rely on the repository client interpreting it correctly. So, I prefer to keep this line.

Hopefully, this whole install: section will become unnecessary relatively soon, and I'll remove it completely in a future Dream release.

Copy link
Member

@mseri mseri Apr 20, 2021

Choose a reason for hiding this comment

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

One last question (from me) before merging. This may be a good opportunity to try how the vendoring behaves. Have you checked what happens if you install dream and then install the opam version of any of its vendored dependencies separately (or if you have some of the vendored dependencies preinstalled when you compile it)?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I just tested removing this line with esy, and esy does not install dream.install by default, causing a dependency error later if the line is removed.

Sorry, dumb question, have you tried removing the whole install section or just that line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One last question (from me) before merging. This may be a good opportunity to try how the vendoring behaves. Have you checked what happens if you install dream and then install the opam version of any of its vendored dependencies separately (or if you have some of the vendored dependencies preinstalled when you compile it)?

Yes, assuming you are asking about opam as the client. Because of the conflicts: section, opam shows prompts like this:

The following actions will be performed:
  ⊘ remove  dream  ~dev* [conflicts with httpaf]
  ∗ install httpaf 0.7.1
===== ∗ 1   ⊘ 1 =====
Do you want to continue? [Y/n]

have you tried removing the whole install section or just that line?

I removed just that line during this PR. The section itself exists because depending on dream fails without it (when the vendored packages are missing), which I experienced when I first tried to install dream after vendoring these deps. I also tried variations of dune install -p ... (as suggested by @kit-ty-kate, thank you) and (vendored_dirs). However, dune install -p did not seem to be able to actually find the vendored packages' .install files when used on its own. (vendored_dirs) disallows passing -p options for selecting subpackages of vendored packages, which forces an overestimation of their deps that are needed for dream, since they are monorepos, pulling in e.g. async via httpaf-async.

So, AFAIK, opam-installer is the only thing that works right now (apart from hacky manual scripts). I've been implicitly testing with opam-installer for several weeks now.

Copy link
Contributor Author

@aantron aantron Apr 20, 2021

Choose a reason for hiding this comment

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

(vendored_dirs) disallows passing -p options for selecting subpackages of vendored packages, which forces an overestimation of their deps that are needed for dream, since they are monorepos, pulling in e.g. async via httpaf-async.

And to clarify this, the failure is during the build of dream itself, since it picks up false transitive dependencies like async, which are not installed (and shouldn't be, on a request for dream only). So I don't know if dune install together with (vendored_dirs) would work, because (vendored_dirs) forces a failure during build, before the install step. EDIT: To the best of my knowledge.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, since you made most of the Piaf dependency choices (my forks of http/af and websocket/af, lwt_ssl, etc), you could depend on Piaf and its vendored libs (piaf.httpaf, piaf.h2, etc).

Then you'd just need to vendor websocket/af and e.g. use a subdir stanza like I do in Piaf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may well do that eventually. However, I understand that then Dream will be "locked" to whatever exact commits Piaf is vendoring, is that right? At this early point, I still need the flexibility to choose other commits, but I hope to streamline that away soon.

Thanks for the subdir stanza tip.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanations, indeed one reason I was asking was due the presence of those conflicts. Thanks for clarifying the issue. It is a pity that it does not install cleanly due to the vendoring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean in the context of a switch with those packages? It seems to install cleanly otherwise. The vendoring is a temporary situation. My goal is to eliminate it relatively quickly. I think it's fine for a first release — this isn't affecting any users of Dream, because it mostly doesn't have any. It will just be a temporary annoyance for early adopters.

@camelus
Copy link
Contributor

camelus commented Apr 20, 2021

Commit: 75bb0cc

A pull request by opam-seasoned @aantron.

☀️ All lint checks passed 75bb0cc
  • These packages passed lint tests: dream.1.0.0~alpha1

☀️ Installability check (+1)
  • new installable packages (1): dream.1.0.0~alpha1

@dinosaure
Copy link
Contributor

@aantron the failure is due to: savonet/ocaml-ssl#70

@aantron
Copy link
Contributor Author

aantron commented Apr 20, 2021

@dinosaure Thanks, subscribed to that issue!

@mseri
Copy link
Member

mseri commented Apr 22, 2021

@aantron I guess it's time to announce the release :)

@mseri mseri merged commit 60f96a6 into ocaml:master Apr 22, 2021
@aantron
Copy link
Contributor Author

aantron commented Apr 22, 2021

@mseri Thanks!

I'm going to hold off announcing it immediately, because now, with the package in opam, I need to do some deployment testing, etc. I may end up making some ~alpha2 release based on any experience from that (hopefully not too soon!), so there's a good chance that's the one that will get an intentional announcement.

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

Successfully merging this pull request may close these issues.

6 participants