xapi-idl: jbuilderize #174

edwintorok · 2017-09-29T16:43:42Z

This has actually been useful for @jjd27 during the clustering work: you can just drop the jbuilderized xapi-idl in a subdirectory and build everything together in planex when you are making changes to the interface.

Caveats: xcp vs xapi-idl differences in project name and library name, I had to create opam packages for both and have them depend on eachother, but maybe there is a better way.

Some libraries still use camlp4 which requires splitting the lib internally into 3 parts: ppx-modules-needed-by-camlp4-module, camlp4-interface, rest of ppx modules. Signed-off-by: Edwin Török <[email protected]>

Signed-off-by: Edwin Török <[email protected]>

These functions/types are declared but not exposed through the mli Signed-off-by: Edwin Török <[email protected]>

%s does not accept a precision Signed-off-by: Edwin Török <[email protected]>

Signed-off-by: Edwin Török <[email protected]>

The tests pass either way, but partition2=partition2 looks like a typo. Signed-off-by: Edwin Török <[email protected]>

these warnings can't easily be fixed Signed-off-by: Edwin Török <[email protected]>

The first bind could actually use bind_1 but that is deprecated too, just use the non-deprecated version in both places. Signed-off-by: Edwin Török <[email protected]>

Signed-off-by: Edwin Török <[email protected]>

Slight problem with how ocamlfind libraries are all called xcp, but the opam package is xapi-idl. Jbuilder expects the two of them to match though. Signed-off-by: Edwin Török <[email protected]>

Based on xapi-project/nbd#bugfix-v2.x Signed-off-by: Edwin Török <[email protected]>

Signed-off-by: Edwin Török <[email protected]>

This should be replaced by String.lowercase_ascii, but that wouldn't compile with 4.02.3 anymore. Asking everyone to upgrade to 4.04.2 might be too early for now. Signed-off-by: Edwin Török <[email protected]>

coveralls · 2017-09-29T17:04:02Z

Changes Unknown when pulling 254f58e on edwintorok:jbuilderize into ** on xapi-project:master**.

mseri

Did you use something particular for b80d419, like a dead code tool, or you followed warnings?

This was a massive work, I had only some minor pedantic comments. Once those are resolved I am happy to approve it

mseri · 2017-10-02T09:56:06Z

lib/syslog.ml

@@ -49,7 +49,7 @@ let facility_of_string s =

 exception Unknown_level of string
 let level_of_string s = 
-	match String.lowercase s with
+	match (String.lowercase s)[@ocaml.warning "-3"] with


Please don't silence these, it is a constant reminder of necessary changes once we drop support for 4.02

good point, I'll try to make the warning non-fatal for jbuilder build --dev instead, so you can still see other warnings and doesn't stop the whole build on this one.

If you can make it, it would be great! However, afaik, the set of dev flags is hardcoded for the moment.

I'd leave that fail (we can always silence it by hand while fixing the dev build) and make the release build the default one.

Looks like jbuilder --dev expands the flags into :standard and then I can turn some errors back into warnings with -warn-error -N: (flags (:standard -w -39 %s -warn-error -3)).
Not sure whether we want --dev to be the default or not though.

mseri · 2017-10-02T10:54:12Z

.travis.yml

@@ -11,6 +11,7 @@ services:
 env:
  global:
    - OCAML_VERSION=4.04.2
+    - PINS="xcp:."


I did a similar thing for stdext and I already start regretting it... Should we instead update all the package references and use xapi-idl once and for all? For the rrd-stuff port I decided to go and update all the packages, it's annoing and invasive but it's a cleanup afterall. I'd like to know @robhoes and @lindig point of view as well.

I've been giving a few thoughts about this. I think for the time being, for libraries like this or stdext that are pervasive across the whole product, we should value backward compatibility over coherence. After we are done with the ports, we can go an kill the backward compatibility hacks all at once.

mseri · 2017-10-02T10:55:22Z

Makefile

-	@ocamlbuild -clean
-	@rm -f setup.data setup.log setup.bin
+reindent:
+	    git ls-files '*.ml' '*.mli' | xargs ocp-indent --syntax cstruct -i


I am going to update the internal documentation using this: it's much cleaner than our default Makefile

mseri · 2017-10-02T10:56:13Z

example/jbuild

+(executable
+ ((name example)
+  (flags (:standard -w -39 %s))
+  (libraries (lwt lwt.unix xcp rpclib))))


Please sort the libraries names

mseri · 2017-10-02T10:56:59Z

lib/jbuild

+  (flags (:standard -w -39 %s))
+  (modules (:standard \ updates task_server scheduler channel_helper))
+  (c_names (syslog_stubs))
+  (libraries (cmdliner uri re cohttp xmlm unix sexplib


It's going to become long but we are sticking to sorted columnar list of dependencies, so please separate them on different lines and sort them.

In this case, in the dependent libraries and executable you may want to first list the xapi-idl dependencies (xcp, xcp.updatex, xcp.xen, ...) and then all the others to emphasize which package libraries are needed where.

Please split them and sort them also in the rest of this file and the other jbuild files

You can probably guess how I'll solve this :)
Just to clarify do you also prefer an empty line between the xcp and the rest:

(libraries (xcp xcp.updates xcp.xen lwt lwt.unix oUnit rpclib threads ))

:) I think they are fine as they are in your new commit

mseri · 2017-10-02T11:04:21Z

xcp.opam

 depends: [
  "ocamlfind" {build}
-  "ocamlbuild" {build}
+  "jbuilder" {build & >= "1.0+beta11"}
  "base-threads"


Let's sort also the dependencies here

mseri · 2017-10-02T11:05:52Z

example/example.ml

@@ -14,7 +14,6 @@

 open Xcp_service
 module D = Debug.Make(struct let name = "example" end)


If we don't use it at all, lets drop this as well

mseri · 2017-10-02T11:05:57Z

lib/updates.ml

@@ -5,7 +5,6 @@ open Stdext
 open Pervasiveext

 module D = Debug.Make(struct let name = "updates" end)


If we don't use it at all, lets drop this as well

mseri · 2017-10-02T11:06:25Z

lib/posix_channel.ml

@@ -92,11 +90,7 @@ let finally f g =
    g ();
    raise e

-let file_descr_of_int (x: int) : Unix.file_descr =
-  Obj.magic x (* Keep this in sync with ocaml's file_descr type *)


Instead of silencing the deprecation warning completely just make the warning non-fatal, even when jbuilder --dev is used. Signed-off-by: Edwin Török <[email protected]>

Signed-off-by: Edwin Török <[email protected]>

edwintorok · 2017-10-02T12:38:12Z

Thanks for the review, I followed jbuilder warnings for the dead code in b80d419.
Before merging I'll have to sort the dependencies like you said, and also update the spec file, which references oasis.

coveralls · 2017-10-02T12:58:40Z

Changes Unknown when pulling f231e9b on edwintorok:jbuilderize into ** on xapi-project:master**.

mseri · 2017-10-02T16:24:00Z

lib/updates.ml

@@ -4,9 +4,6 @@
 open Stdext


This is using stdext, although stdext is not listed as a dependency (it probably comes as a transitive one). Can you please remove all the open Stdext and replace the use of the library with the minimal subset of new sublibraries? In this case it would be Xapi_stdext_pervasives.Pervasiveext and I believe replacing it with let finally = Xapi_stdext_pervasives.Pervasiveext.finally should be enough in this case

And there is a reference to Xapi_stdext_threads.Threadext half way through.

Pushed, I also needed Xapi_stdext_monadic. The finally was not needed, when I added the open for Pervasiveext.

mseri · 2017-10-02T16:32:12Z

Other mentions of stdext that I've seen in xcp-idl are:

Xapi_stdext_date in

xcp-idl/storage/storage_interface.ml

Line 84 in da3aa11

snapshot_time = Stdext.Date.to_string Stdext.Date.never;
Xapi_stdext_thread and Xapi_stdext_pervasives in https://github.com/xapi-project/xcp-idl/blob/7febc10d2df59225b6f3f7ad2837941a037ccefe/lib/task_server.ml
xcp-idl/lib/xcp_service.ml

Line 14 in 7060017

open Stdext

(that seems to possibly not need it at all)

I think it would be good to move from the stdext dependency to the three specific xapi-stdext-? packages only where needed as part of this port.

Use sorted columnar format for dependencies. Reindent jbuild files Signed-off-by: Edwin Török <[email protected]>

coveralls · 2017-10-03T08:52:32Z

Changes Unknown when pulling 933d514 on edwintorok:jbuilderize into ** on xapi-project:master**.

Signed-off-by: Edwin Török <[email protected]>

coveralls · 2017-10-03T10:07:58Z

Changes Unknown when pulling ebe9f3f on edwintorok:jbuilderize into ** on xapi-project:master**.

Xapi_stdext_monadic is needed for Opt Signed-off-by: Edwin Török <[email protected]>

edwintorok · 2017-10-03T10:21:34Z

Not sure why Travis can't find xapi-stdext-threads I see it in my opam.

mseri · 2017-10-09T08:16:17Z

We found that the build issue is an issue with ocaml-docker used in the tests. @edwintorok submitted a PR to fix it ocaml/ocaml-ci-scripts#188 and should not prevent this PR to be merged. If the fix is not approved in a reasonable time, I will send a PR to move travis back to ocaml-travis.

edwintorok added 15 commits September 29, 2017 17:40

Port to jbuilder

fcc7337

Some libraries still use camlp4 which requires splitting the lib internally into 3 parts: ppx-modules-needed-by-camlp4-module, camlp4-interface, rest of ppx modules. Signed-off-by: Edwin Török <[email protected]>

fix jbuild --dev warnings for unused values

e71f678

Signed-off-by: Edwin Török <[email protected]>

jbuild --dev: drop unused opens

cb4d2a5

Signed-off-by: Edwin Török <[email protected]>

jbuilder --dev: drop unused code

b80d419

These functions/types are declared but not exposed through the mli Signed-off-by: Edwin Török <[email protected]>

jbuilder --dev: fix printf format

83e63b7

%s does not accept a precision Signed-off-by: Edwin Török <[email protected]>

jbuilder --dev: fix deprecated String usage

82be0ac

Signed-off-by: Edwin Török <[email protected]>

jbuilder --dev: fix warning on free

4b7922f

Signed-off-by: Edwin Török <[email protected]>

jbuilder --dev: fix typo in test

0e51b61

The tests pass either way, but partition2=partition2 looks like a typo. Signed-off-by: Edwin Török <[email protected]>

jbuilder --dev: disable warnings for some pieces of code

89afe85

these warnings can't easily be fixed Signed-off-by: Edwin Török <[email protected]>

Switch to non-deprecated Lwt_unix.Versioned.bind_2

e7af869

The first bind could actually use bind_1 but that is deprecated too, just use the non-deprecated version in both places. Signed-off-by: Edwin Török <[email protected]>

jbuilder --dev: fix ambiguos doc comment

d28be23

Signed-off-by: Edwin Török <[email protected]>

jbuilder: xapi-idl vs xcp

d9c3009

Slight problem with how ocamlfind libraries are all called xcp, but the opam package is xapi-idl. Jbuilder expects the two of them to match though. Signed-off-by: Edwin Török <[email protected]>

Add .coverage.sh

9fad115

Based on xapi-project/nbd#bugfix-v2.x Signed-off-by: Edwin Török <[email protected]>

Add bisect_ppx preprocessing

ad4c135

Signed-off-by: Edwin Török <[email protected]>

Disable deprecation warning on String.lowercase

254f58e

This should be replaced by String.lowercase_ascii, but that wouldn't compile with 4.02.3 anymore. Asking everyone to upgrade to 4.04.2 might be too early for now. Signed-off-by: Edwin Török <[email protected]>

mseri requested changes Oct 2, 2017

View reviewed changes

edwintorok added 2 commits October 2, 2017 13:33

fixup! Disable deprecation warning on String.lowercase

75d2189

Instead of silencing the deprecation warning completely just make the warning non-fatal, even when jbuilder --dev is used. Signed-off-by: Edwin Török <[email protected]>

drop unused D module

f231e9b

Signed-off-by: Edwin Török <[email protected]>

mseri reviewed Oct 2, 2017

View reviewed changes

Reformat jbuild files

933d514

Use sorted columnar format for dependencies. Reindent jbuild files Signed-off-by: Edwin Török <[email protected]>

sort opam dependencies

ebe9f3f

Signed-off-by: Edwin Török <[email protected]>

Move from deprecated Stdext to more specific Xapi-stdext-* modules

f14bc49

Xapi_stdext_monadic is needed for Opt Signed-off-by: Edwin Török <[email protected]>

mseri approved these changes Oct 3, 2017

View reviewed changes

mseri merged commit 03f01c8 into xapi-project:master Oct 9, 2017

edwintorok deleted the jbuilderize branch October 9, 2017 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xapi-idl: jbuilderize #174

xapi-idl: jbuilderize #174

edwintorok commented Sep 29, 2017

coveralls commented Sep 29, 2017 •

edited

Loading

mseri left a comment

mseri Oct 2, 2017

edwintorok Oct 2, 2017

mseri Oct 2, 2017

edwintorok Oct 3, 2017

mseri Oct 2, 2017

mseri Oct 3, 2017

mseri Oct 2, 2017

mseri Oct 2, 2017

mseri Oct 2, 2017

edwintorok Oct 3, 2017

mseri Oct 3, 2017

mseri Oct 2, 2017

mseri Oct 2, 2017

mseri Oct 2, 2017

mseri Oct 2, 2017

edwintorok commented Oct 2, 2017

coveralls commented Oct 2, 2017 •

edited

Loading

mseri Oct 2, 2017

mseri Oct 2, 2017

edwintorok Oct 3, 2017

mseri commented Oct 2, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading

edwintorok commented Oct 3, 2017

mseri commented Oct 9, 2017

		@@ -14,7 +14,6 @@

		open Xcp_service
		module D = Debug.Make(struct let name = "example" end)

		@@ -5,7 +5,6 @@ open Stdext
		open Pervasiveext

		module D = Debug.Make(struct let name = "updates" end)

xapi-idl: jbuilderize #174

xapi-idl: jbuilderize #174

Conversation

edwintorok commented Sep 29, 2017

coveralls commented Sep 29, 2017 • edited Loading

mseri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwintorok commented Oct 2, 2017

coveralls commented Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mseri commented Oct 2, 2017 • edited Loading

coveralls commented Oct 3, 2017 • edited Loading

coveralls commented Oct 3, 2017 • edited Loading

edwintorok commented Oct 3, 2017

mseri commented Oct 9, 2017

coveralls commented Sep 29, 2017 •

edited

Loading

coveralls commented Oct 2, 2017 •

edited

Loading

mseri commented Oct 2, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading