Skip to content

Dune port, split into two opam packages#7

Merged
hannesm merged 12 commits intomirage:masterfrom
avsm:dune-port
Jan 14, 2019
Merged

Dune port, split into two opam packages#7
hannesm merged 12 commits intomirage:masterfrom
avsm:dune-port

Conversation

@avsm
Copy link
Copy Markdown
Member

@avsm avsm commented Jan 6, 2019 •
edited
Loading

This also removes depopts and has a separate arp-mirage opam package.

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Jan 6, 2019

thanks, why [wip]? anything on your side that needs to be done before merge?

@avsm avsm changed the title [wip] Dune port Dune port, split into two opam packages Jan 6, 2019
@avsm
Copy link
Copy Markdown
Member Author

avsm commented Jan 6, 2019

I just wanted to check the travis results. I've also updated the CHANGES file now, so it should be good for release. You can use dune-release tag && dune-release bistro for a pleasant experience if you haven't tried that before :)

@avsm avsm mentioned this pull request Jan 6, 2019
Closed
@samoht
Copy link
Copy Markdown
Member

samoht commented Jan 6, 2019

You can use dune-release tag && dune-release bistro

Or just You can use dune-release tag && dune-release if you want to save a few keystrokes :-)

@avsm
Copy link
Copy Markdown
Member Author

avsm commented Jan 8, 2019

This should be good to go @hannesm. The only regression is (I think) the benchmarking trick of copying the other arp implementation from tcpip. But since we're planning on deleting that anyway, just benchmarking this library should be fine.

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Jan 9, 2019

ok, I tried this PR. Notes:
(a) in arp.opam, ppx_bisect should be bisect_ppx (why is this not covered by travis?)
(b) When I locally pin arp and arp-mirage, the resulting META does not contain a "version" anymore, is this intentional?
(c) The META of arp depends on bisect_ppx.runtime -- this should not be the case. I also dislike to depend on bisect_ppx unconditionally, maybe a Makefile target that adds bisect_ppx.runtime to the dune file and the pps would be better? Do other projects using dune have a smooth solution for coverage / bisect?
(d) There is no longer a description in the META file (well, that's fine with me)

In order to move forward, we need to fix (a) and (c). Is (b) and (d) intentional? I noticed that mirage/mirage-types/mirage-types-lwt/mirage-runtime also no longer have a version in their META (while mirage-net/mirage-net-lwt still have some version) -- is this a regression (or intentional) between jbuilder and dune?

hannesm
hannesm reviewed Jan 9, 2019
View reviewed changes
hannesm
hannesm reviewed Jan 9, 2019
View reviewed changes
hannesm
hannesm reviewed Jan 9, 2019
View reviewed changes
hannesm
hannesm reviewed Jan 9, 2019
View reviewed changes
hannesm
hannesm reviewed Jan 9, 2019
View reviewed changes
avsm added 4 commits January 10, 2019 23:36
@avsm
opam files: add synopsis, doc dir for dune-release, fix bisect pkg
c4a21c8 
review comments from @hannesm in mirage#7
@avsm
make Makefile coverage removal more precise
abb584c 
review comments from @hannesm in mirage#7
@avsm
travis: make arp-mirage a depopt and install main package
c18946f 
needs ocaml/ocaml-ci-scripts#228 for multiple packages to be
fixed properly
@avsm
make bisect an optional build if the BISECT_ENABLED variable is set
63826d0 
there is a proper fix for this being worked on in ocaml/dune#57

review comment from @hannesm in mirage#7 to not depend on bisect runtime
unconditionally in production uses of arp
@avsm
Copy link
Copy Markdown
Member Author

avsm commented Jan 10, 2019

a) not sure why travis didn't pick up the bad package name...
b) yeah the version will show up in release packages when dune-release is run (see dune package versions)
c) I've made the bisect conditional on BISECT_ENABLE being set in 63826d0 - there is a proper fix in dune being worked on in ocaml/dune#57
d) the META now sets a description in 63826d0

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Jan 14, 2019

thanks @avsm - I'll have to take a second look at this tonight and will merge + transfer to mirage organisation. I think I'll mark the bisect_ppx dependency as {with-test} -- does this make sense?

@avsm
Copy link
Copy Markdown
Member Author

avsm commented Jan 14, 2019

sounds good. I dont think bisect is a with-test dependency, since it will break the build if the environment variable is set and a normal build is done. It shouldn't do any harm to leave as a build depend I think...

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Jan 14, 2019

I transferred ownership to mirage organisation, and pushed two more commits. plan to merge once CI passes

@hannesm hannesm merged commit 09229b3 into mirage:master Jan 14, 2019
avsm added a commit to avsm/opam-repository that referenced this pull request Feb 2, 2019
@avsm
[new release] arp and arp-mirage (1.0.0)
d872b71 
CHANGES:

* split opam package into two separate ones: a core
  `arp` package and the `arp-mirage` implementation
  for MirageOS that has more dependencies.  This
  eliminates the use of depopts that was done previously
  to build the Mirage layer. (mirage/arp#7 @avsm)

* port build system to Dune (mirage/arp#7 @avsm). The `make coverage`
  and `make bench` targets will do the job of the previous
  topkg targets for those.

* minor fixes to ocamldoc comments to be compatible with
  odoc.

* use mirage-random and mirage-random-test instead of a
  nocrypto dependency in tests and bench (mirage/arp#7 @hannesm)

* import tests from mirage-tcpip (mirage/arp#8 @hannesm)

* depend on the ethernet opam package, no longer provided
  by tcpip >3.7.0 (mirage/arp#9 @hannesm)
@avsm avsm mentioned this pull request Feb 2, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hannesm hannesm hannesm left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@avsm @hannesm @samoht