-
Notifications
You must be signed in to change notification settings - Fork 11
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
Modern build #8
base: master
Are you sure you want to change the base?
Modern build #8
Conversation
dbm.opam
Outdated
] | ||
depends: ["jbuilder" {build}] | ||
depexts: [ | ||
["libgdbm-dev"] {os-distribution = "debian"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your are using opam2 syntax here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to opam-1.2 syntax
dbm.opam
Outdated
build: [ | ||
["./configure"] | ||
["jbuilder" "build" "-p" name "-j" jobs] | ||
["jbuilder" "build" "testdbm.exe"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should probably me more in a separate build-test
field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the stardard way of doing tests with jbuilder seems to be jbuilder runtest -p name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks for educating me.
By the way, funnily, jbuilder runtest -p dbm, will only run tests after a fresh build. I.e. only once.
This might be a jbuilder bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, I do I tell opam to run those tests now?
Apparently they are not run by a regular install of the package.
@@ -0,0 +1,25 @@ | |||
opam-version: "1.2" | |||
maintainer: "https://github.com/ocaml/opam-repository/issues" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Xavier should be marked as maintainer I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure he wants to be bothered
Poke @rgrinberg in case there is something that the new dune configurator can do here. |
I just pushed several commits. Hope it helps. |
dbm.opam
Outdated
["jbuilder" "build" "-p" name "-j" jobs] | ||
] | ||
build-test: [ | ||
["jbuilder" "runtest" "-p" name] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should pass the jobs parameter here as well.
Configurator can certainly simplify things here, but i think the first cut here that maintains a configure script is fine. I recall trying to port cryptokit to dune earlier and @xavierleroy wasn't too happy. |
There's a lot to say here, at the risk of sounding like an ungrateful old curmudgeon. Contributions from outside a core set of developers/maintainers are essential for free software project. This is doubly true for legacy code like this DBM binding, where the maintainers are not original developers nor regular users and do maintenance as a service to the community. Contributions of the form "before I contribute anything you must switch to my favorite tool B instead of your favorite tool A" are more problematic. I'm not familiar with your tool B. Switching to it means that I'll lose my ability to keep maintaining, if I can't find the time and patience to learn your tool B. In return you make no commitment that you'll take over the maintenance of the part of the project now written in B. So, why should I accept your contribution? How will it make my life as a by-default-maintainer easier? Now, speaking of JBuilder/Dune more specifically, I'm in a "wait and see" position. Having used makefiles for nearly 30 years, I do realize they are a pain for medium to large projects. I find them quite appropriate for small, stupid projects like this DBM library. JBuilder/Dune is certainly a great tool for Jane Street-sized code bases, for which it was designed, but feels like overkill for such a trivial task. Finally you do realize that Dune/JBuilder has been in beta for several years and hasn't reached 1.0 yet, let alone adopted its definitive name? To me this screams "don't use yet". |
Xavier, You will not lose anything currently: make -f Makefile.old will still work as it has done before. The very cute sh configure script, with its on-the-fly generation
I can certainly maintain this part of the configure script. Also, I guess @kit-ty-kate and @rgrinberg will land a helpfull hand
Personally, I don't wait and see, I try and see. I think I have tried all OCaml build systems over the years. jbuilder is quite good and actively maintained by someone with deep pockets Previously, I was favoring obuild, but obuild is not backed by any company Regards, |
I think this is a fair position. (we've been developing it for a year or so now, and it has been rapidly iterating). There is one feature which makes Dune really nice when used in combination with opam, and that is the ability to clone multiple repositories in a directory and build them directly via a toplevel build. The resulting operations are fast and incremental, unlike (e.g.) an However, these PRs to port libraries piecemeal do not reveal that benefit since this only works if a good transitive cut of libraries work are ported to Dune. I'm working (with @rgrinberg @diml @dra27) on a 'duniverse' repository that can add Dune overlays to projects that do not support it yet, and provides a base to evaluate build times and flexibility vs sticking with Makefiles. Once that is done, we should have concrete data to evaluate PRs like this and justify the cost of moving to a new tool... |
@@ -92,6 +92,29 @@ echo | |||
echo "Configuration successful" | |||
echo | |||
|
|||
# jbuilder config file generation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach is the worst of all worlds however :-) Generating jbuild files at configuration time means that build composition doesn't work, and incremental builds also suffer. Using the configurator library gets around that by making the flag probing part of the build itself.
To shed light on the naming and status, we have implemented the renaming to Dune in the git repository and are preparing the 1.0.0 release of Dune.
Ah, that's an interesting comment. This is quite the opposite actually. Jane Street first developed Jenga as a super build system able to handle huge code bases, but it wasn't practical for the open source world. Jbuilder started a leaner version of Jenga + the Jane Street build rules that was meant to make it easy to build Jane Street packages in the opam world, so it is definitely aimed at open source projects. At this point I think Dune is actually a smaller dependency than make for OCaml projects, given that the only requirement to build and use Dune is a working OCaml distribution. In addition to what @avsm said, I would say that the good practices for building and installing projects tend to change over time. For instance, at some point it became standard to build and install cmxs files, then it became standard to install cmt/cmti files, etc... With Dune, the change is made once and for all in Dune and all projects immediately benefit from it since the Dune metatada are very high-level. With hand-written build systems, every individual project need to be updated. |
As a side effect, this does install the ocamldoc so that ocp-browser can find it.