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

misc: develop against vendored dependencies #462

Merged
merged 1 commit into from
Feb 6, 2023

Conversation

bikallem
Copy link
Contributor

This PR allows the dev repo to support developing, testing and debugging against vendored dune dependencies.

/cc @hannesm

dune Outdated Show resolved Hide resolved
@bikallem bikallem force-pushed the vendored-dependencies branch 2 times, most recently from 44eb1ef to 9dd5214 Compare December 22, 2022 10:00
.gitignore Outdated
random/
vendor/
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need this any more either...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm ... git seems to want to include things in this folder and it always lists it when doing git status. Of course you can do selective git add ... but mostly I prefer the convenience of git add --all. This prevents from mistakenly adding things from vendor dir to this repo.

@hannesm
Copy link
Member

hannesm commented Dec 22, 2022

Since there's quite some discussion about "vendoring" and "dependencies", is that somewhere documented what needs to be done and how to address issues? Is there a general move in the OCaml and opam ecossytem to add at random places "vendor"? I'm not a huge fan, especially since I've a hard time understanding what this "vendoring" is trying to solve, and how it works workflow-wise (including upstreaming diffs, etc.).

@@ -10,6 +10,7 @@
server-ec.pem
server-ec.key
(package tls-eio)
(package mirage-crypto-rng-eio)
Copy link
Member

Choose a reason for hiding this comment

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

this change looks like an actual independent bugfix - which is fine to be merged in a separate commit (and PR)

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 think this is only necessary in a vendoring situation like this PR, thus the change. Outside of the use-case, i.e. vendoring your dependencies - this is not needed as before this PR.

@hannesm
Copy link
Member

hannesm commented Dec 22, 2022

Likely there exist some "vendoring" documentation, so maybe @bikallem or @avsm would mind to point to that? Is this "opam-monorepo" a part of it (or "opam lock")? I'm still puzzled, but maybe I just settled on a very arcane development style. Would love to hear how you people are working and which tools you're using.

@bikallem
Copy link
Contributor Author

The vendoring approach used here is documented here https://dune.readthedocs.io/en/stable/dune-files.html#vendored-dirs-since-1-11.

I am using vendoring here mainly so that I can insert 'printf traces/Eio.traceln' to tls-eio dependencies located in 'vendor' dir. This PR makes working with such vendored dependencies a little bit precise.

@bikallem bikallem force-pushed the vendored-dependencies branch from 9dd5214 to e688c08 Compare December 23, 2022 22:23
@hannesm
Copy link
Member

hannesm commented Feb 4, 2023

Since @talex5 contributed the "eio/tests/dune", and my knowledge of mdx and dune is rather limited, I'd like to hear his opinion whether this PR is reasonable. Thanks.

Copy link
Contributor

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Looks good to me. When used without vendoring, the opam metadata will ensure that mirage-crypto-rng-eio is available, but if dune is building that at the same time then it will need to know about the dependency.

@hannesm hannesm merged commit f8fac5c into mirleft:main Feb 6, 2023
@bikallem bikallem deleted the vendored-dependencies branch February 6, 2023 11:25
hannesm added a commit to hannesm/opam-repository that referenced this pull request Feb 14, 2023
CHANGES:

* BREAKING: new opam package tls-lwt (formerly tls.lwt), in dune:
  (libraries tls.lwt) should now be libraries (tls-lwt)
  (mirleft/ocaml-tls#468 @hannesm, reported mirleft/ocaml-tls#449 by @mbacarella)
* tls: update to mirage-crypto 0.11 API (mirleft/ocaml-tls#468 @hannesm)
* tls: relax SignatureAlgorithms extension handling to allow OpenSSL
  interoperability tests with TLS 1.0 and TLS 1.1 (mirleft/ocaml-tls#469 @hannesm)
* tls: remove Utils.filter_map and and Utils.option, use Stdlib instead (mirleft/ocaml-tls#455
  @hannesm)
* tls: do not globally open Utils (mirleft/ocaml-tls#455 @hannesm)
* tls: export log source of Tracing module (mirleft/ocaml-tls#461 @bikallem)
* tls: remove unused ciphersuites to reduce binary size (mirleft/ocaml-tls#467 @hannesm)
* tls-lwt: do not catch out of memory exception (mirleft/ocaml-tls#469 @hannesm)
* tls-eio: add fuzz testing using crowbar (mirleft/ocaml-tls#456 mirleft/ocaml-tls#463 @talex5)
* tls-eio: update to eio 0.7 (mirleft/ocaml-tls#456 @talex5)
* tls-eio: fix test for develop with vendoring (mirleft/ocaml-tls#462 @bikallem)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants