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

Switch build system to Dune #10

Closed
wants to merge 4 commits into from
Closed

Switch build system to Dune #10

wants to merge 4 commits into from

Conversation

dra27
Copy link
Contributor

@dra27 dra27 commented Aug 15, 2018

Working recently on the features which led to #5, #8, #9, and also verbosemode/syslog-message#17 I experienced first-hand how suboptimal developing with opam pins is for someone contributing across several libraries.

As a retrospect to the work, I offer a switch of build system to Dune - as is so often the case, the diff deletes more than it adds. I've checked that the build is equivalent, at least as far as findlib is concerned. Dune is somewhat opinionated about the names of library archives, so they have to have underscores rather than hyphens, but this should have no effect since the findlib package name is unchanged. There is one niggle, since Dune doesn't allow a module to be included in multiple archives: I have packaged Logs_syslog_lwt_common in a logs-syslog.lwt.common package. This is not ideal, as it exposes it - the alternative is to have Logs_syslog_mirage_common and have rules which automatically copy the ml and mli file (@diml, @rgrinberg - I think that's the best solution at the moment?)

I've written about the experience here, and I hope the comparison may provide persuasion, if needed, for why Dune is a good upgrade.

I haven't used it, but please note @samoht's dune-release tool, which I believe provides everything which topkg-care did before.

@ghost
Copy link

ghost commented Aug 15, 2018

This is not ideal, as it exposes it - the alternative is to have Logs_syslog_mirage_common and have rules which automatically copy the ml and mli file (@diml, @rgrinberg - I think that's the best solution at the moment?)

yes

@rgrinberg
Copy link

Also, you should perhaps comment on the proposal here ocaml/dune#1017 and see how well it addressed your concerns.

@hannesm
Copy link
Owner

hannesm commented Aug 16, 2018

thanks for this, @dra27. I've three questions about this PR:

  • (version 0.1.1) in dune-project is the version number of logs-syslog? this should not be there, if it is needed it should be %%VERSION%% (otherwise I've to manually change it on every release)
  • how do I generate documentation (I used to use topkg doc and topkg publish doc - does this workflow still work with dune?)
  • did you compare the META file to the dune-generated one? are they equal (apart from the - vs _ and the additional logs-syslog.lwt.common sublib)?

@dra27
Copy link
Contributor Author

dra27 commented Aug 16, 2018

@rgrinberg - possibly, though to be honest it feels more like here there's a (trust-me-I-really-mean-to-include-this-module-twice Logs_syslog_lwt_common) stanza needed, which would be simpler than private libraries. This module is included in two libraries which should never both be used at once - but I guess the problem is that findlib doesn't provide a way of indicating that one package conflicts another.

@rgrinberg
Copy link

In that case, you can simply setup a copy rule on the module source. Only if it's really the case that both libraries are mutually exclusive.

@dra27
Copy link
Contributor Author

dra27 commented Aug 16, 2018

@hannesm - there's a similar question about the version in dune-project on the syslog-message PR. I'm not certain what the echt approach is here*, so I'll get back to you (putting the version there is one way to communicate the version to META). Dune can generate docs with odoc (dune build @doc) and I think from the docs that dune-release publish then pushes those.

I did check META through, yes - there are two other differences:

  1. Because META is generated, Dune doesn't use exists_if, it simply omits the package specification if it wasn't compiled
  2. I allowed dependencies to propagate transitively so, for example, logs-syslog.unix doesn't directly depend on syslog-message, it picks that up via depending on logs-syslog

* TL;DR My personal opinion on versions is that they belong in the repo, not the history (i.e. the tag is for the convenience of finding the correct source tree) and so dune-release or whatever should be tagging the project based on the version in dune-project and then doing a semver bump immediately after where, if you have maintenance branches, it does a minor version bump (cf. OCaml) or otherwise a revision version bump, but that's just my opinion!

@dra27
Copy link
Contributor Author

dra27 commented Aug 17, 2018

OK, it's possible that the version should go in dune-project in future, but for now dune-release does the necessary thing.

@dra27
Copy link
Contributor Author

dra27 commented Aug 17, 2018

The last commit there is a way of eliminating the lwt.common package, which restores the previous layout of the Logs_syslog_lwt_common module in both logs_syslog_lwt and logs_syslog_mirage. In the absence of Dune private libraries, it requires that the Logs_syslog_lwt_common module is referred to by its "internal" name.

@hannesm
Copy link
Owner

hannesm commented Sep 17, 2018

@dra27 thanks, and sorry for the delay. now that dune 1.2.0 is out, which includes support for private modules ocaml/dune#1241, any chance you could rebase this PR and use them? :)

@hannesm
Copy link
Owner

hannesm commented Oct 27, 2018

(sorry about my confusion in the latest comment, the private modules feature in dune does not cover the use case here, but @rgrinberg pointed to ocaml/dune#1017 which we can use once it is adapted and implemented).

I rebased this PR and #11 on master in #12 -- thanks for your work!

@hannesm hannesm closed this Oct 27, 2018
hannesm added a commit to hannesm/opam-repository that referenced this pull request Oct 27, 2018
CHANGES:

- support for syslog-message.1.0.0
  it split the `message` field of Syslog_message.t into `tag` and `content`
  use the name of Logs.src as tag when sending messages
- move build system to dune (hannesm/logs-syslog#10 by @dra27)
- provide Logs_syslog.facility Logs.Tag.def to specify facility in log
  message, add ?facility as default facility to all reporters (reported in hannesm/logs-syslog#7,
  fixed in hannesm/logs-syslog#9 by @dra27)
- append ':' to source (reported in hannesm/logs-syslog#6, fixed in hannesm/logs-syslog#8 by @dra27)
- add missing dependency on unix for logs-syslog.unix (hannesm/logs-syslog#4 by @dra27)
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.

3 participants