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

Weak dependencies should be recorded even if not present #6843

Closed
vicuna opened this issue Apr 17, 2015 · 6 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link

commented Apr 17, 2015

Original bug ID: 6843
Reporter: @lpw25
Assigned to: @gasche
Status: closed (set by @xavierleroy on 2017-02-16T14:14:54Z)
Resolution: fixed
Priority: normal
Severity: minor
Fixed in version: 4.02.2+dev / +rc1
Category: ~DO NOT USE (was: OCaml general)
Related to: #6998

Bug description

The weak dependencies (i.e. modules which are aliased but not used in -no-alias-deps mode) are recorded in the cmi file. This makes it easier to produce deterministic builds when warning 49 is enabled -- by allowing a build system to ensure that removing a file which is weakly depended on will cause a rebuild, and thus an error.

However, if warning 49 is disabled weak dependencies are only recorded if the file exists, which makes deterministic builds much harder.

The attached patch ensures that weak dependencies are always added, even if the file is not present.

The other option would be not to add weak dependencies when warning 49 is disabled, but then the build output would depend on the set of warnings chosen.

File attachments

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 26, 2015

Comment author: @gasche

What I understand from reading the patch is that you move an add_import call before a filename lookup that could fail (and thus not add_import in some cases). I don't understand how the control flow would depend on whether or not Warning 49 is enabled. Would you care to explain why this change fixes this problem?

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 26, 2015

Comment author: @lpw25

What I understand from reading the patch is that you move an add_import call before a filename lookup that could fail (and thus not add_import in some cases). I don't understand how the control flow would depend on whether or not Warning 49 is enabled.

Sorry, in the description when I say "enabled" and "disabled" referring to warning 49 I mean "enabled as an error" and "not enabled as an error".

When warning 49 is not an error the failure of the filename lookup is ignored, which means that compilation continues even though add_import has not been called.

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 26, 2015

Comment author: @gasche

Based on your explanation, I've uploaded another patch (relying on the indempotence of add_import) that I think is clearer. If you don't have strong feeling that the previous approach is better, I'll merge this one.

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 26, 2015

Comment author: @gasche

Is it important that this change be in 4.02? If not, it will be trunk-only.

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 30, 2015

Comment author: @lpw25

The new patch seems fine, although I personally think that the new patch is clearer as a patch whilst the old patch results in clearer resulting code (at least if you borrowed the comment from the new patch) since it keeps management of imports within env.ml.

Is it important that this change be in 4.02?

It is hard to quantify importance. It is relatively important since it does produce non-determinism in builds based on -no-alias-deps. Not just theoretically but in practice.

@vicuna

This comment has been minimized.

Copy link
Author

commented May 1, 2015

Comment author: @gasche

Merged in trunk and 4.02 -- I mixed the two changes.

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.