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
Comments
Comment author: ertai Can you provide us with the dependencies between these modules? |
Comment author: @ygrek No dependencies (files can be simply empty) : |
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 I would suggest you to change one of the names. |
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). |
Comment author: kerneis I have been bitten by this issue as well: http://sourceforge.net/p/cil/bugs/138/ 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). |
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! |
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. |
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. |
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. |
Comment author: kerneis Here is a patch implementing your option (3), only slightly improved and simplified. See commit message in patch for details. |
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. |
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. |
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. |
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. |
Comment author: @damiendoligez ocamlbuild is now a separate project that lives on GitHub. |
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
The text was updated successfully, but these errors were encountered: