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

Fix a make -j bug, by ensuring Lazy depends on CamlinternalLazy. #2148

Merged
merged 1 commit into from Nov 18, 2018

Conversation

Projects
None yet
3 participants
@stedolan
Copy link
Contributor

commented Nov 14, 2018

Without this patch, make -j often fails to build the stdlib with a message along the lines of:

no cmx file was found in path for module CamlinternalLazy

The issue is that stdlib files that use lazy actually depend on camlinternalLazy.cmi because matching.ml expands lazy pattern matches to code that refers to CamlinternalLazy. However, since this dependency does not appear in the source code, there is no way for ocamldep to discover it. This means that when building the stdlib, there is no constraint ensuring that CamlinternalLazy is built before stdlib modules using Lazy.

This causes issues with parallel make, but the issue can be reproduced using a sequential make invocation:

cd stdlib
make clean
make stdlib_stream.cmo

This patch adds a dependency on CamlinternalLazy into lazy.mli. Its presence makes ocamldep see that all files that use Lazy depend on camlinternalLazy.cmi.

@stedolan stedolan force-pushed the stedolan:make-parallel-lazy branch from aa4111b to 44224ee Nov 14, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

Thanks for tracking the issue down. The fix you propose is fragile with respect to -no-alias-deps; could we instead have an initialization function in CamlinternalLazy

let init[@inline never] () = ()

called by Lazy?

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

That's a good point! I don't think the suggested fix is enough, though: we need to make lazy.cmi depend on camlinternalLazy.cmi. Making lazy.cmo depend on camlinternalLazy.cmi is not sufficient, so the change will have to be something that mentions CamlinternalLazy in the mli file.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

The dependency needs to be made obvious at the interface level, right?

@nojb

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

What about adding a type alias 'a t = 'a lazy_t to CamlinternalLazy and type 'a t = 'a CalminternalLazy.t = 'a lazy_t in Lazy ?

@stedolan stedolan force-pushed the stedolan:make-parallel-lazy branch from 44224ee to 971ae22 Nov 14, 2018

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

Thanks, that works.

@gasche

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

Sorry for being slow here, but I don't follow. If code generated by the compiler uses CamlinternalLazy, that creates implementation dependencies, not interface dependencies. Why do we need an interface-dependency between the two modules?

My current partial-answer to this question is: what we need is not a dependency from Lazy to CamlinternalLazy, but an implementation dependency from lazy-using modules to CamlinternalLazy. Because implementation-dependencies are not necessarily transitive (separate compilation), we use an interface dependency instead. But then: aren't native-implementation dependencies transitive?

(Bonus question: are we in trouble for classes as well?)

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

The file stream.ml uses a construct that expands to reference camlinternalLazy. In order to compile this file, producing stdlib__stream.cmo, the file camlinternalLazy.cmi (representing the interface to CamlinternalLazy) must be available. Later on, in order to link, the implementation must also be available, but that is not the issue here.

What we need is a dependency from the implementation of any lazy-using modules to the interface of camlinternalLazy. That is, since stream.ml uses laziness, stdlib__stream.cmo must depend on camlinternalLazy.cmi. Since stdlib__stream.cmo already depends on stdlib__lazy.cmi, this patch achieves the dependency by making sure stdlib__lazy.cmi depends on camlinternalLazy.cmi.

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

I'm not sure about classes! This issue was discoved in the multicore CI (which always does parallel builds). The CI regularly broke due to camlinternalLazy, but I haven't seen class-related breakage.

@gasche

gasche approved these changes Nov 14, 2018

Copy link
Member

left a comment

I find the interface-level solution slightly unsatisfying, but I understand that it fixes a real problem and I'm not exactly sure what a better alternative would be, so I'm approving the PR and proposing to merge (if the CI passes) and move on.

@gasche

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

(One thing I don't like very much is exposing internal mentions in the .mli that people read for documentation. But this is acceptable for Lazy because the definition is internal/builtin magic anyway.)

@gasche

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

I think this could benefit from a Changes entry, in the chance that (1) someone encounters the problem later and looks in Changes for an indication of fixedness or (2) the new approach troubles someone someday and they look for the culprit.

@stedolan stedolan force-pushed the stedolan:make-parallel-lazy branch from 971ae22 to 5b61a50 Nov 14, 2018

Fix a make -j bug, by ensuring Lazy depends on CamlinternalLazy.
Without this patch, make -j often fails to build the stdlib with
a message along the lines of:

    no cmx file was found in path for module CamlinternalLazy

The issue is that stdlib files that use `lazy` actually depend on
camlinternalLazy.cmi because matching.ml expands lazy pattern
matches to code that refers to CamlinternalLazy. However, since
this dependency does not appear in the source code, there is no
way for ocamldep to discover it. This means that when building
the stdlib, there is no constraint ensuring that CamlinternalLazy
is built before stdlib modules using Lazy.

This causes issues with parallel make, but the issue can be
reproduced using a sequential make invocation:

    cd stdlib
    make clean
    make stdlib_stream.cmo

This patch adds a dependency on CamlinternalLazy into lazy.mli.
Its presence makes ocamldep see that all files that use Lazy also
depend on camlinternalLazy.cmi.

@stedolan stedolan force-pushed the stedolan:make-parallel-lazy branch from 5b61a50 to 122c521 Nov 14, 2018

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

Changes entry added. The build failure was due to line numbers changing in one of the backtrace tests. I've just updated these, the next build should pass.

@gasche gasche merged commit f9e1c09 into ocaml:trunk Nov 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.