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

build and install annotation files #54

Merged
merged 4 commits into from
Oct 10, 2014
Merged

build and install annotation files #54

merged 4 commits into from
Oct 10, 2014

Conversation

madroach
Copy link
Contributor

@madroach madroach commented Oct 4, 2014

Hi,
it would be nice if oasis was able to build and install annotation files.
Is this sensible default behaviour? Do we need a knob to turn it off?

@kit-ty-kate
Copy link
Member

Maybe you can put it in an AlphaFeature but I think it good like that. This PR is really nice ! I'm all for it.

@avsm
Copy link
Member

avsm commented Oct 4, 2014

+1 for cmt/cmti files -- there should be no real downside except for this not being available on earlier OCaml revisions (3.12.1).

OASISFileUtil.file_exists_case
(List.map
(Filename.concat path)
(make_fnames modul [".annot";"cmti";".cmt"]))
Copy link
Member

Choose a reason for hiding this comment

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

.cmti ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ocamlc -bin-annot -c intf.mli will produce intf.cmi and intf.cmti

Copy link
Member

Choose a reason for hiding this comment

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

There's a dot missing in the "cmti" string above

@gildor478
Copy link
Member

The .annot is only available since 3.11.0, but I suppose this is totally fine and OASIS needs > 3.11 anyway.

Also, I would like to have at least one test for this PR!

@madroach
Copy link
Contributor Author

madroach commented Oct 5, 2014

I don't know how the tests work.
Could you provide a simple test case with two modules A and B which are linked with Pack: true into a library C. this should then produce {a,b,c}.cmt.
I implemented and tested against 24 janestreet libraries: core, async, ...
at least core is packing its submodules into a core.cmo before building the library core.cma.

@gildor478
Copy link
Member

You can create a _oasis + required files in test/data/TestOCamlbuild/annot (several directories already exist) and then create a test to check your expectation in test/TestOCamlbuild.ml.

For example, you can look at:
https://github.com/ocaml/oasis/blob/master/test/TestOCamlbuild.ml#L175

Thanks

Christopher Zimmermann added 2 commits October 6, 2014 07:32
use ocamlbuild builtin bin_annot and annot tags instead of rolling our own.

also remove some trailing whitespace
@madroach
Copy link
Contributor Author

madroach commented Oct 6, 2014

Turns out the existing tests already cover the cases I planned to test. So I ended up just adding the annotation files to files expected to be installed by the current tests. On OpenBSD I now get the following test results:

Ran: 231 tests in: 179.82 seconds.
FAILED: Cases: 231 Tried: 231 Errors: 0 Failures: 22 Skip:  6 Todo: 0 Timeouts: 0.
..
Ran: 2 tests in: 0.17 seconds.
OK

The test logs are here: ftp://gmerlin.de/pub/oasis/

Also I found the builtin bin_annot and annot tags in ocamlbuild and enabled them instead of adding custom -annot and -bin-annot flags. Problem is that ocamlbuild is not yet using -bin-annot when packing (http://caml.inria.fr/mantis/view.php?id=6599).
Oasis seems to be fault tolerant enough. Running the tests with an unpatched ocamlbuild will just tell me differences: -lib/ocaml/packedlib/packedlib.cmt. This means that oasis did successfully install all the rest and just skipped the missing packetlib.cmt, doesn't it?

@madroach madroach closed this Oct 6, 2014
@madroach madroach reopened this Oct 6, 2014
@gildor478
Copy link
Member

The test suite fails at the point of packedlib.cmt.

Not a big deal.

Can you just remove this specific files and add a comment so that I can re-add it with the right logic when ocamlbuild will be released with the patch ?

gildor478 added a commit that referenced this pull request Oct 10, 2014
build and install annotation files
@gildor478 gildor478 merged commit 46f4671 into ocaml:master Oct 10, 2014
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