ocaml / dune

A composable build system for OCaml.
https://dune.build/
MIT License
1.63k stars 408 forks source link

Dune always sets -no-alias-deps for all files #1819

Open garrigue opened 5 years ago

garrigue commented 5 years ago

@ejgallego kindly ported lablgtk to dune, and his PR is now merged. However, I only realized after that that it now requires to add -linkall for all lablgtk3 applications. Indeed, the low-level layer of lablgtk is composed of modules containing initialization code, and they reexport some automatically generated modules. As an example, GtkBin reexports the Frame module of GtkBinProps. However, if GtkBin itself is not linked, the necessary initialization in Gtk is not done, and it crashes at runtime. Is there a way to disable this use of -no-alias-deps ? Note that to truly work, this should be an inherited attribute: i.e., all projects using lablgtk3 should not use -no-alias-deps, except for files that do not run any code. More generally, -no-alias-deps was never intended to be the default mode...

garrigue commented 5 years ago

The related discussion is in a separate PR about example compilation.

ghost commented 5 years ago

Hmm, I don't think we can do that, -no-alias-deps is required to implement wrapping of libraries, so we can't disable it for all projects using lablgtk3.

What do you think of explicitly marking such link time dependency? For instance by adding this in gtkBin.ml:

let () = Frame.linkme
garrigue commented 5 years ago

-no-alias-deps is only needed when you compile the wrapper. This is sufficient to break the transitivity for all libraries that rely on it (but you must then keep the wrapper in the library, since other objects will depend on it). See the Makefile.build in the tool-ocamldep test in the testsuite for that.

I have implemented a workaround for the lack of dependency in Lablgtk, but the problem is that it means that every time you move a project to dune, you force an unneeded change of semantics onto it. It's unfortunate that nobody saw that before. (The fact that it's actually unneeded)

ghost commented 5 years ago

That's what I implemented at the very beginning, however it led to some weird issues so I enabled it everywhere. I can't remember what the issues were though. I can see that we also compile everything with -no-alias-deps inside Jane Street. /ccing @lpw25 and @sliquister in case they remember why we do that.

I agree that this situation is unfortunate.

nojb commented 5 years ago

FTR, in the manual https://caml.inria.fr/pub/docs/manual-ocaml/extn.html#sec250 it says to compile all files (not just the wrapper) with -no-alias-deps (that's what I am doing in https://github.com/ocaml/ocaml/pull/2218).

garrigue commented 5 years ago

I don't quite remember why it is written that way in the manual. Maybe for simplicity. Also it avoids having to include an implementation for the wrapper itself. It would be nice if you could give a try at using it only on the wrapper, since this would clarify the situation. I didn't try that approach on many examples.

garrigue commented 5 years ago

Actually, it seems that I'm the author of the explanation, which was written just after introducing module aliases, in June 2014. The tool-ocamldep-modalias example was written 1.5 years later, in December 2015, and implements both approaches, checking that they are properly supported by ocamldep, and that linking also works.

ghost commented 5 years ago

This discussion makes me think that it would be simpler if there was no choice and the behaviour was settled once and for all.

From the point of view of the user, it's hard to know what language constructions will force the link of other modules. For instance type t = M.t won't but exception E = M.E will, which might not be obvious at all for users.

garrigue commented 5 years ago

By the way, the problem in Lablgtk was not that GtkBin wouldn't depend on GtkBinProps, but the other way round: that GBin accessed GtkBinProps.Frame through GtkBin, but doesn't depend anymore on GtkBin itself, which contained the initialization code. This is a side-effect of -no-alias-deps, which doesn't add dependencies to intermediate modules. The workaround is to move the the initialization to GtkBinProps (and all other props files), but this is Lablgtk specific.

garrigue commented 5 years ago

Well, I remember people exploiting this already a long time ago to have code and type dependencies go in opposite directions: compile the .mlis and .mls in opposite order. For a long time, external declaration didn't introduce dependencies either. Currently only type-level features do not introduce link-time dependencies (i.e., type or module type), with module aliases being a pure type level feature only with -no-alias-deps. This is the other way round with extensible types: their tags being concrete values, using a tag from an extensible type declaration introduces a dependency. This could be better documented. And some people actually argue that all accesses to types should actually induce dependencies (which would make sense if we are to introduce some value components in types someday).

ghost commented 5 years ago

I'm not surprised that these details have been exploited in the past :) For Dune it makes more sense to make the choice once and support it well. Supporting well all possible combinations is a lot more work and makes reasoning about all this a lot harder.

Personally, I think we should make -no-alias-deps the default and deprecate this option in the compiler. That would avoid surprises. Would that seem reasonable to you?

garrigue commented 5 years ago

Changing the default always create surprises. Test, test and test. And these initialization problems only appear at runtime. This is the reason we were so careful about not changing the default behavior for a laxer one.

garrigue commented 5 years ago

I suppose that another way to handle is just to say that since dune makes it the default, and prevents you from disabling it, automatically all projects using dune are -no-alias-deps compatible. Then you just have to wait until everybody uses dune. Honestly, this is the only practical solution I see.

ghost commented 5 years ago

That seems good to me

garrigue commented 5 years ago

However, it would be nice to document it somewhere. I've looked at the dune documentation, and it is not mentioned anywhere that -no-alias-deps is used, and that you cannot override it. A few explanations on how to work around the change in behavior would be welcome too. Would have to test, but the simplest one for all situations seems to be to replace all module aliases module M = P by module M = struct include P end. If you just want to ensure that the aliased module is brought in by the aliasing one, you can add a value dependency as you suggested. The approach I used in Lablgtk is a bit different, but there were just too many aliases.

ghost commented 5 years ago

Agreed. I suppose we could have a "migrating to dune" section in the manual to document this and maybe other subtleties

v-gb commented 5 years ago

It's better to have -no-alias-deps everywhere because if you don't, then if you create the main file for your library by writing:

module A = A module B = B let x = C.x

then you're creating a library that links everything all the time.

garrigue commented 5 years ago

This sounds like a bug. If you compile this single file with -no-alias-deps, then it shouldn't depend on A and B. Or did somebody add some extra code?

v-gb commented 5 years ago

(and of course this main file could be defined by including a module defined in another file that contains aliases)

I'm just saying this kind of example is why you want -no-alias-deps for every file, not just the wrapper. For the wrapper it's required to build at all, and for every other files, it avoids the bad exe size of -pack.

v-gb commented 5 years ago

For labltgtk, you may be able to undo the effect of -no-alias-deps with (library_flags -linkall), instead of having -linkall the whole exe? That's how I changed libraries when they had forward-reference-using-refs or other weird things.

garrigue commented 5 years ago

OK, you're arguing that -no-alias-deps is a better default in all cases, not just for wrapper files. I was just making the point that if the goal is just to be able to wrap a library, then using -no-alias-deps on the wrapper only is sufficient. So we're discussing different problems. As discussed with @diml, the only question is documenting things properly.

garrigue commented 5 years ago

But I don't want to use -linkall ! It means including everything, just what you're trying to avoid with -no-alias-deps. You should never suggest using -linkall, this is bound to be a bad idea. It is only useful for toplevels, or applications which run dynamic code.

jhwoodyatt commented 5 years ago

I referenced this issue in a discussion about considering Dune for my Orsetto project.