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.

Sign up for GitHub

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

Jump to bottom

[ocaml] Fix compilation with 4.09.0 #2342

Closed
wants to merge 2 commits into from
Closed

[ocaml] Fix compilation with 4.09.0 #2342

wants to merge 2 commits into from

Conversation

ejgallego
Copy link
Collaborator

This seems fairly strange, it seems 4.09.0 has somehow modified its
let generalization rules?

I will follow up with a PR for adding 4.09 support to the CI [while we discuss this one]

@ejgallego
[ocaml] Fix compilation with 4.09.0
ed85cfe 
This seems fairly strange, it seems 4.09.0 has somehow modified its
`let` generalization rules?
@ghost
Copy link

ghost commented Jul 2, 2019

That's unexpected indeed. /cc @lpw25 @trefis

ghost
ghost reviewed Jul 2, 2019
View reviewed changes
src/watermarks.ml
@@ -28,7 +28,6 @@ let make_watermark_map ~name ~version ~commit =
match Opam_file.get_field opam_file name with
| None -> Error (sprintf "variable %S not found in opam file" name)
| Some value ->
let err = Error (sprintf "invalid value for variable %S in opam file" name) in
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about let err () = ...? That would avoid the duplication

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that, but likely we should drop this PR and see if this is fixed upstream.

PR for trunk testing coming soon.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, waiting a little bit to see how it plays out upstream seems good

@rgrinberg
Copy link
Member

octachron said this is a bug with OCaml: ocaml/ocaml#8779

@lpw25
Copy link

lpw25 commented Jul 2, 2019

To be clear, it's not really a bug it's a change in unspecified behaviour. Hiding a cmi file is only partially supported and that is what is causing the problem here.

@ghost
Copy link

ghost commented Jul 3, 2019

To sum up a discussion we had yesterday with @aalekseyev, @lpw25 and @trefis:

Dune is currently providing an (implicit_transitive_deps false) feature that is not properly backed up by the compiler. This is not ideal and we should be more careful about such things in the future. Adding proper support for it in the compiler seems easy enough and we can reasonably expect it to land in 4.10. Proper support would take the form of supporting "hidden" includes that could be used by the compiler but not the user. For instance, passing -Ih <libdir>/core_kernel (assuming the new flag is called -Ih) would allow the compiler to read core_kernel.cmi but the user wouldn't be able to use the Core_kernel module in their code.

Such a change would make this feature more robust, with the disadvantage that we would loose the speedup provided by the current implementation of (implicit_transitive_deps false). We could recover it to some extent by reducing the number of include flags by reading .cmi files. At this point, it's not clear how hard or if it is even theoretically possible to support the feature as currently implemented in dune, i.e. only passing include flags for dependencies mentioned in the dune file but not for transitive dependencies.

@ghost
Copy link

ghost commented Jul 17, 2019

Let's merge this now. We can always revert it if the compiler patch is merged and included in 4.09. @ejgallego could you sign your commit?

@rgrinberg
Copy link
Member

I applied Jeremie's fix instead.

@rgrinberg rgrinberg closed this Jul 17, 2019
@ejgallego ejgallego deleted the trunk_let branch July 17, 2019 13:22
@ejgallego
Copy link
Collaborator Author

Sounds good, thanks folks!

1 similar comment
@ejgallego
Copy link
Collaborator Author

Sounds good, thanks folks!

@ejgallego
Copy link
Collaborator Author

I'm much afraid that the build is still broken in 4.09, this time in lib/src.ml due to some other recent commits :(

ejgallego added a commit to ejgallego/dune that referenced this pull request Jul 20, 2019
@ejgallego
Fix build with OCaml trunk.
5b4df2a 
Another instance of what is detailed in
ocaml#2342 ; until
ocaml#2298 is fixed I guess we'll have
to live with commits like this.
ejgallego added a commit to ejgallego/dune that referenced this pull request Jul 20, 2019
@ejgallego
Fix build with OCaml trunk.
d583f51 
Another instance of what is detailed in
ocaml#2342 ; until
ocaml#2298 is fixed I guess we'll have
to live with commits like this.

Signed-off-by: Emilio Jesus Gallego Arias <e@x80.org>
@ejgallego ejgallego mentioned this pull request Jul 20, 2019
Merged
ejgallego added a commit to ejgallego/dune that referenced this pull request Jul 21, 2019
@ejgallego
Fix build with OCaml trunk.
752f035 
Another instance of what is detailed in
ocaml#2342 ; until
ocaml#2298 is fixed I guess we'll have
to live with commits like this.

Signed-off-by: Emilio Jesus Gallego Arias <e+git@x80.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ejgallego @rgrinberg @lpw25 @jeremiedimino