Skip to content

Conversation

@bartoszmodelski
Copy link
Contributor

@bartoszmodelski bartoszmodelski commented Dec 23, 2022

This sets benchmarks as optional and puts them into a different new package (which includes yojson dependency).

With this change:

  • installing lockfree (opam install ./lockfree.opam --deps-only) does not install yojson
  • pipelines still work as they install all packages in the repo (something like opam install . --deps-only I suspect)

I decided to make the benchmarks package a superset of lockfree (so contributor can just install lockfree-with-benchmarks to get everything) but I'm happy to do it in a different way if there's a reason.

@bartoszmodelski bartoszmodelski marked this pull request as ready for review December 23, 2022 18:05
@bartoszmodelski
Copy link
Contributor Author

Example in Irmin - they seem to follow the alternative approach

@lyrm lyrm force-pushed the mark-yojson-optional branch from a0abd0c to 7fcba29 Compare January 23, 2023 10:05
@polytypic
Copy link
Contributor

I don't know what the recommended approach is with dune/opam, but I wonder whether making benchmarks tests (instead of ordinary executables) would be a suitable solution? (And then the dependencies would be with-test.)

@lyrm
Copy link
Collaborator

lyrm commented Jan 25, 2023

That would mean that benchmarks run with opam runtest. I am not sure it is a good idea. Other opinions are welcomed :)

@polytypic
Copy link
Contributor

Just noting that having benchmarks also function as tests is not necessarily bad. That may help with making sure that both the library works and that the benchmarks also work. Just need to make the default parameters such that running the benchmarks doesn't take a lot of time.

@bartoszmodelski
Copy link
Contributor Author

Sounds like it would work, but a bit less customary?

@Sudha247
Copy link
Collaborator

yojson and alcotest have since been marked as test-only dependencies in #76. Thanks for the efforts to find what turned out to be a reasonable solution. Closing this PR.

@Sudha247 Sudha247 closed this Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants