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 to jbuilder, prepare to release 1.1.0 #7

Closed
wants to merge 8 commits into from

Conversation

djs55
Copy link
Member

@djs55 djs55 commented May 19, 2017

No description provided.

djs55 added 3 commits May 19, 2017 17:39
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
@hannesm
Copy link
Member

hannesm commented May 19, 2017

I've a couple of questions:

  • why rename a mli to ml?
  • how is the documentation supposed to be generated now?
  • what's the purpose of mirage-net-lwt.version and mirage-net.version? which process is supposed to update them on a new release? manually?

@avsm
Copy link
Member

avsm commented May 19, 2017

Beta10 of jbuilder (the next one) is due to have an Isco target

@avsm
Copy link
Member

avsm commented May 19, 2017

Sorry, an "odoc" target :-)

@@ -0,0 +1 @@
1.1.0
Copy link
Member

Choose a reason for hiding this comment

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

this is not needed with beta9 which adds a new jbuilder subst CLI option to do exactly the same watermarking as topkg does.

Copy link
Member

Choose a reason for hiding this comment

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

(so you can continue to use %%VERSION%% in various places)

src/jbuild Outdated
(public_name mirage-net)
(libraries (bytes result mirage-device))
(modules (Mirage_net))
(flags (:standard -w "A-4-41-44" -warn-error "+1..49" -safe-string))
Copy link
Member

@samoht samoht May 19, 2017

Choose a reason for hiding this comment

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

I'm not sure about adding warning errors to the build runes run which will be run after the release. I thing it's better to use topkg build --dev while doing development (which has a sane set of warning/error options).

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case I coped the contents of the _tags file: true: warn_error(+1..49), warn(A-4-41-44). I'm not very familiar with the semantics of this though. Is it not safe to use a specified list "1,2,3,...,49" since they're already defined and won't be redefined in future versions of the compiler? I thought the main problem was with the equivalent of -Wall which includes future yet-to-be-defined errors.

Wrt to topkg could you explain a bit more? I see this:

$ topkg build --dev
topkg: unknown option `--dev', did you mean `-d' ?
Usage: topkg build [OPTION]... [BUILD_CONF]...
Try `topkg build --help' or `topkg --help' for more information.

(I was never a topkg expert). Is your suggestion to use topkg and jbuilder together? Could you outline what the build and release workflow would look like? Thanks!

@samoht
Copy link
Member

samoht commented May 19, 2017

@hannesm: the "ml -> mli" renaming is due to ocaml/dune#9
@djs55 if we want to keep the mli, it seems that the proper way to do it is to add an explicit copy rule: mirage/ocaml-github#192 (comment)

I have no strong opinion on whether we should keep the mli or have a ml file, but pure mlis don't work with module aliases which seems a good argument to me to not use them (until this is fixed in the compiler).

@hannesm
Copy link
Member

hannesm commented May 19, 2017 via email

djs55 added 4 commits May 20, 2017 09:18
The command `jbuilder subst` should be used to perform the substitution
before creating the release tarball.

Signed-off-by: David Scott <dave@recoil.org>
The PACKAGE is set to mirage-net-lwt which depends on mirage-net
(also from this repository) which we PIN.

Signed-off-by: David Scott <dave@recoil.org>
This trick is copied from

https://github.com/ocaml-doc/odoc/blob/master/odoc.opam

Signed-off-by: David Scott <dave@recoil.org>
This copies the configuration from

  https://github.com/ocaml-doc/odoc/blob/master/pkg/pkg.ml

It requires the new `topkg-jbuilder` package:

  https://github.com/ocaml/opam-repository/pull/9231/files

Signed-off-by: David Scott <dave@recoil.org>
@djs55
Copy link
Member Author

djs55 commented May 20, 2017

I've pushed a few more changes for review (can squash them later)

  • .version files now contain %%VERSION%%. Note that jbuilder understands several ways to find the version number to put in the generated META file -- it can also take the version from the opam file. My understanding is that opam-repository repository opam files are not supposed to contain the version so I opted for not putting the version in here either, so that I could manually diff and cp the opam files. I'm not sure what opam-publish does or prefers.
  • I copied a trick from odoc itself: the build rules now perform jbuilder subst if the package is {pinned}
  • I also copied the odoc package's pkg/pkg.ml file which references the new package topkg-jbuilder. After I installed this (opam pin add topkg-jbuilder git://github.com/diml/topkg-jbuilder.git since topkg-jbuilder.0.1.0 - via opam-publish ocaml/opam-repository#9231 is not yet merged) I could then run topkg build
  • I verified that odoc compile --help says Input file (either .cmti or .cmt). I did a odoc compile and then odoc html to check that stuff was output -- so I think we're good with either the .ml or the mli file

BTW have we documented the ideal / best-practice package release workflow anywhere? I've still been relying on the github "release" button which I realise isn't ideal, and I haven't got a great way to automatically update API docs etc.

@samoht
Copy link
Member

samoht commented May 20, 2017 via email

@samoht
Copy link
Member

samoht commented May 20, 2017 via email

Signed-off-by: David Scott <dave@recoil.org>
@@ -0,0 +1 @@
%%VERSION%%
Copy link
Member

Choose a reason for hiding this comment

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

I am still not totally sure why we need this file :-)

@djs55
Copy link
Member Author

djs55 commented May 29, 2017 via email

@avsm
Copy link
Member

avsm commented May 30, 2017

oops i am clashing with #8

@djs55
Copy link
Member Author

djs55 commented May 30, 2017

@avsm I'm happy to close this in favour of yours.
@samoht I think I now agree that the .version file is unnecessary -- somewhere topkg and jbuilder picked up the right version and put it in the META file.

@djs55 djs55 closed this May 30, 2017
@avsm
Copy link
Member

avsm commented May 30, 2017

@djs55:

@samoht I think I now agree that the .version file is unnecessary

waaaaaaaat i added all those version files for nothing??

@hannesm hannesm mentioned this pull request Jun 5, 2017
djs55 added a commit to djs55/mirage-net that referenced this pull request Jun 5, 2017
See discussion on mirage#7

Signed-off-by: David Scott <dave@recoil.org>
djs55 added a commit to djs55/mirage-net that referenced this pull request Jun 5, 2017
See discussion on mirage#7 and mirage#8

Signed-off-by: David Scott <dave@recoil.org>
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