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

creates incomplete cma under some circumstances #5185

Closed
vicuna opened this issue Nov 30, 2010 · 16 comments
Closed

creates incomplete cma under some circumstances #5185

vicuna opened this issue Nov 30, 2010 · 16 comments

Comments

@vicuna
Copy link

vicuna commented Nov 30, 2010

Original bug ID: 5185
Reporter: @ygrek
Status: resolved (set by @damiendoligez on 2017-02-27T13:12:41Z)
Resolution: suspended
Priority: normal
Severity: minor
Version: 3.12.0
Target version: later
Category: -for ocamlbuild use https://github.com/ocaml/ocamlbuild/issues
Tags: patch
Related to: #6067
Monitored by: @gasche kerneis @hcarty

Bug description

Consider the following mllib:
X Q Y
Create in the current directory empty files x.ml and y.ml AND q.mli
Under normal circumstances ocamlbuild will notice missing implementation of Q module and build fails.
But if the name of the mllib file matches the name of one of the modules inside it - build succeeds with incomplete cma - e.g. with y.mllib we get:

$ PATH=/opt/ocaml-3.12.0/bin:$PATH ocamlbuild -classic-display y.cma
/opt/ocaml-3.12.0/bin/ocamldep.opt -modules x.ml > x.ml.depends
/opt/ocaml-3.12.0/bin/ocamldep.opt -modules q.mli > q.mli.depends
/opt/ocaml-3.12.0/bin/ocamlc.opt -c -o q.cmi q.mli
/opt/ocaml-3.12.0/bin/ocamldep.opt -modules y.ml > y.ml.depends
/opt/ocaml-3.12.0/bin/ocamlc.opt -c -o x.cmo x.ml
/opt/ocaml-3.12.0/bin/ocamlc.opt -c -o y.cmo y.ml
/opt/ocaml-3.12.0/bin/ocamlc.opt -a y.cmo -o y.cma

File attachments

@vicuna
Copy link
Author

vicuna commented Nov 30, 2010

Comment author: ertai

Can you provide us with the dependencies between these modules?

@vicuna
Copy link
Author

vicuna commented Nov 30, 2010

Comment author: @ygrek

No dependencies (files can be simply empty) :
$ touch x.ml y.ml q.mli
$ echo X Q Y > y.mllib
$ ocamlbuild -classic-display y.cma
/usr/bin/ocamldep.opt -modules x.ml > x.ml.depends
/usr/bin/ocamldep.opt -modules q.mli > q.mli.depends
/usr/bin/ocamlc.opt -c -o q.cmi q.mli
/usr/bin/ocamldep.opt -modules y.ml > y.ml.depends
/usr/bin/ocamlc.opt -c -o x.cmo x.ml
/usr/bin/ocamlc.opt -c -o y.cmo y.ml
/usr/bin/ocamlc.opt -a y.cmo -o y.cma

@vicuna
Copy link
Author

vicuna commented Nov 30, 2010

Comment author: ertai

OK, got it I think it makes not much sense to call the library the same name as a module given the two rules:

.mllib -> .cma
.cmo (and its deps) -> .cma

I would suggest you to change one of the names.

@vicuna
Copy link
Author

vicuna commented Nov 30, 2010

Comment author: @ygrek

Hm, I have several libraries where cma name matches the name of one of source files. I don't think it is a good option to enforce such constrain, especially as it works ok usually. Moreover if ocamlbuild starts making arbitrary limitations on naming of modules it will become less and less useful. This bug itself is not critical at all - the problem gets noticed and missing implementation is added - but it leads to confusion. All in all I think this should be considered as a bug and not an incorrect usage - cause it is a common pattern in libraries (name of the module equal to the name of the library).

@vicuna
Copy link
Author

vicuna commented Jun 27, 2013

Comment author: kerneis

I have been bitten by this issue as well: http://sourceforge.net/p/cil/bugs/138/
In my case the situation was even worse: some .cmo were packed together in the .cma, but not all of them (so it seemed to work until you wanted to use the missing modules).

I agree with ygrek that it is a bug: ocamlbuild should check in the .cmo -> .cma rule that no corresponding .mllib exists, and fail if it is the case (to make sure the .mllib -> .cma rule is used, and detects missing implementations).

@vicuna
Copy link
Author

vicuna commented Jul 1, 2013

Comment author: keithw

I have also just been bitten by this (4.00.1) and it was very mystifying and took me a long time to figure out that I merely had a module listed in .mllib tht didn't exist in the file system. Any error message would be good!

@vicuna
Copy link
Author

vicuna commented Jul 30, 2013

Comment author: kerneis

I've attached a patch to fix the issue. It has been tested successfully on the error case mentionned in the original bug report, as well as on my own issue with CIL.

@vicuna
Copy link
Author

vicuna commented Jul 30, 2013

Comment author: @dbuenzli

Maybe we should drop that cmo -> cma rule no ? It seems to bring a lot of confusion (see also PR #5208).

@vicuna
Copy link
Author

vicuna commented Aug 5, 2013

Comment author: @gasche

After a discussion with kerneis, I believe the patch is ok but it may be worth wondering wherelse such treatment would be useful, and if we're not papering over some design issues that would be profitably fixed. Feel free to ping again here if there hasn't be noise in about a week, so that at least the patch gets integrated.

Daniel, I don't think dropping the rule entirely is a good idea compatibility-wise. Furthermore, if people rely on this behavior for .cmxa and .cmxs (you seem to use it yourself for .cmxs in the mentioned PR), they will expect it for .cma as well. Better documentation is certainly called for.

@vicuna
Copy link
Author

vicuna commented Aug 6, 2013

Comment author: @gasche

I thought more about the issue. I suspect the path set by this patch would lead us in the design quagmire of "anti-dependencies".

Morally, what we want to say is that in this rule, the %.cma depends on %.cmo and on (NOT %.mllib): the rule should fail in presence of a .mllib. If we were to add such things in ocamlbuild, it would make sense to add it in the general case. Let's consider why this is not such a good idea.

The current implementation by kerneis works well if we assume that .mllib is written by the user and present in the source directory. Otherwise (if .mllib was to be generated by rules, such as a sed preprocessing pass or a copy_rule), to ascertain that no .mllib is present we would have to try to build it, and check that we fail. Anti-dependencies would be dependencies that we try to build, check that they fail and then remove (Prolog's notion of negation by finite failure). The performance implications of this are not good in the general case. In particular, if you cancel the production of the anti-dependency that (unexpectedly) succeeded, you don't cache its result, and you're going to rebuild it again at the next activation of this rule with anti-dependency. Looks like a very dangerous path, to say the least.

I think we should rather try to keep things positive (instead of expressing negation). We could try to guarantee, not that there is a single possible rule for each production, but a form of confluence: that all rules producing a target will give a semantically equivalent result. This is the case for rules producing .cmi for example. It is not the case for rules producing .cma, which is causing the present issue.

I would consider three ways out that I think are good long-term. I'm not sure any of them works, yet.

(1) forbid the use of a .mllib of the same name as a .cmo, to rule out the non-confluence scenario; but users want to preserve that use-case

(2) remove the .cmo -> .cma rule (and corresponding rule for .cmx and .cmxs), forcing people to write .mllib explicitly when they want to; our dear users would certainly also complain

(3) replacing the rule .cmo -> .cma by a rule .cmo -> .mllib producing the trivial single-module .mllib. Not being strongly familiar with the ocamlbuild internals, I'm not sure that will work as expected (eg. not try to do anything when a .mllib (even wrong) already exists), but sometimes we have good surprises.

@vicuna
Copy link
Author

vicuna commented Aug 6, 2013

Comment author: kerneis

Here is a patch implementing your option (3), only slightly improved and simplified. See commit message in patch for details.

@vicuna
Copy link
Author

vicuna commented Aug 6, 2013

Comment author: @gasche

I like the resulting patch a lot, thanks!

I'm a bit wary of it introducing unwanted regressions, though. I wouldn't consider it for inclusion before we have added coverage in the testsuite for all the use-cases affected by this change.

I understand the reason for moving from ".cmo -> .mllib" to ".ml -> .mllib", but it changes the fact that the modification to the can-build graph is local and self-contained. If we want to ensure that we didn't change this (imaginary) graph, we know need to make sure that all .cmo are produced by a .ml (and likewise for .cmx, etc.). In fact you assume the invariant that all "compilation units" in the OCaml sense are manifested by a .ml file at some point in ocamlbuild's process. This is true in the cases that immediately come to mind (.mly produces an intermediate .ml), is it true in the general case? May this assumption break in the future?

PS: the idea of ".cmo -> .mllib" in fact comes from Didier Rémy, with which I discussed the issue this morning.

@vicuna
Copy link
Author

vicuna commented Aug 13, 2013

Comment author: kerneis

Now I have an issue with my patch: it's not as trivial to implement as I thought because it doesn't pull the full transitive closure for mllib, whereas it should.
Working on it currently.

@vicuna
Copy link
Author

vicuna commented Aug 13, 2013

Comment author: @gasche

After more investigation with Gabriel Kerneis, we discovered that the obvious idea of having foo.cmo or foo.ml generate a foo.mllib with a single line in it, "Foo", does not quite work. The reason for this is that the .cmo -> .cma rule, as complained about in #5208, does not only include the module Foo in foo.cma, but also the other .cmo it depends on, transitively -- but not .cma coming from external libraries. Meanwhile, .mllib have an exact semantics: they will produce .cma that only contain the compilation units listed in the file, and not their dependencies.

We agree this is not optimal and violates the principle from least surprise. If we want to generate a .mllib from the .cmo (or .ml) that respects the current semantics, we have to first compute the set of dependencies to include, and write them all in the .mllib file. kerneis is looking into that, but that will require some more work -- and time.

There is the question of whether that exact semantics of .mllib is desirable. It could be more convenient for users to have the transitive closures of dependencies computed for them (as the .cmo -> .cma rules currently does), but I'm not very happy with the idea of breaking backward semantics. Another option would be to have a .inferred.mllib target that would allow users to regenerate the full transitive closure whenever they want, to avoid writing or maintaining the .mllib completely manually.

PS: we also found out that pulling "cmo -> mllib" back into "ml -> mllib" doesn't quite work, because foo.mlpack files give a counterexample of compilation units to which no foo.ml file corresponds.

@vicuna
Copy link
Author

vicuna commented Aug 14, 2013

Comment author: kerneis

And then, we can add a rule .inferred.mllib -> .mllib (a trivial cp), which would automatically be used only if there is no existing mllib file, and solve nicely the original issue.

Note that what is needed to implement .inferred.mllib is probably not very different from (although not exactly the same as) this other issue: #6067. I'll try and think about a solution to both of them.

@vicuna
Copy link
Author

vicuna commented Feb 27, 2017

Comment author: @damiendoligez

ocamlbuild is now a separate project that lives on GitHub.
PR transferred to ocaml/ocamlbuild#166

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant