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 #19

Merged
merged 5 commits into from
Aug 29, 2018
Merged

Switch build system to Dune #19

merged 5 commits into from
Aug 29, 2018

Conversation

dra27
Copy link
Contributor

@dra27 dra27 commented Aug 15, 2018

Working recently on the features which led to #17 and also hannesm/logs-syslog#5, hannesm/logs-syslog#8 and hannesm/logs-syslog#9 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.

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.

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

I have some small additions but as someone who filed such PRs I am happy someone lese did the work of converting to dune 👍

.gitignore Outdated
_build
syslog-message.install
src/.merlin
test/.merlin
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense to just ignore all .merlin files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I have little opinion on that!

dune-project Outdated
@@ -0,0 +1,3 @@
(lang dune 1.1)
(name syslog-message)
(version 0.0.2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be moved to a Changelog file which can be then used by dune-release to create the proper tarballs for releases.

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 don't understand what you mean - dune-project is a Dune system file, so move what?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I'm on a train with bad connectivity.

What I meant was to remove the version information from the dune-project file and instead move it to the CHANGELOG.md, which is picked up by dune-release and used for tags, releases and release changelogs by dune-release. This avoids duplicating the version information and forgetting to update it in one place and also simplifies releases and opam-repository PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha - ultimately the plan is that dune-project becomes the source of truth for the version and it gets propagated elsewhere, but at the moment dune-release does everything that's needed, so I've removed it.

@verbosemode
Copy link
Owner

I think we can also get rid of top.ml, since I've only created it for convenient manual testing of the code in the repl. It seems like Dune has a builtin for this.

@dra27
Copy link
Contributor Author

dra27 commented Aug 17, 2018

@verbosemode - it is indeed supposed to, although it's not working for me at the moment, so I've left top.ml for now!

Use `dune utop src` instead.
@verbosemode
Copy link
Owner

Nice blog post and thanks again for contributing.

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