Skip to content

Conversation

mseri
Copy link
Contributor

@mseri mseri commented Feb 23, 2018

This moves the executables and tests in different scopes, reorganising and clarifying the dependencies.
This is a first step, further improvements will be to further refine the dependencies among those folders and replace stdext with the appropriate xapi-stdext- submodules.

(flags (:standard -bin-annot %s -warn-error +a-3-4-6-9-27-28-29-52))
(modules (:standard \ xapi_main quicktest))
(libraries (
alcotest
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also remove this?

@gaborigloi
Copy link
Contributor

Will it be easier to merge #3468 first and then update this PR to move the new / modified files to their correct place?
Or I guess we should merge this first if reviewing those PRs would take long , and then #3468 will have to be updated.

@mseri
Copy link
Contributor Author

mseri commented Feb 23, 2018

Yes, it will not be a problem to rebase after your changes in #3468. I think that should go first, once it passes travis tests. I am quite surprised that for some reasons #3468 reduces coverage... :(

@coveralls
Copy link

coveralls commented Feb 23, 2018

Coverage Status

Coverage increased (+0.007%) to 17.645% when pulling 8198d64 on mseri:split-things-properly into 7fe5dbd on xapi-project:master.

@mseri
Copy link
Contributor Author

mseri commented Feb 23, 2018

Rebased on top of #3468.

ls ocaml/xapi now fits in a full-screen terminal 🎉 😨

(action (run ${<}))
)
)
|} (flags rewriters) coverage_rewriter
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only for coverage, as far as I know.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I think we could remove flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is still needed until we port rpc to jbuilder. A port is ready but there are some issues with camlp4 and the way it interacts with ppx in xapi-idl, so I'd rather wait until the port is complete

Copy link
Contributor

@minishrink minishrink left a comment

Choose a reason for hiding this comment

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

Yay sensible ordering!

Marcello Seri added 11 commits February 27, 2018 10:27
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
includes exporting the xapi library as an internal (and not installed) xapi_internal library

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
Signed-off-by: Marcello Seri <marcello.seri@citrix.com>
@mseri
Copy link
Contributor Author

mseri commented Feb 27, 2018

I've rebased to avoid the conflicts. I am going to merge this if Travis is happy

@mseri
Copy link
Contributor Author

mseri commented Feb 27, 2018

The xenserver-build-env failure is due to idl changes not yet propagated. The Travis build with xs-opam has successfully passed. Merging

@mseri mseri merged commit c7db7fc into xapi-project:master Feb 27, 2018
@mseri mseri deleted the split-things-properly branch February 27, 2018 10:51
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.

5 participants