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

Generate .cmt files in native compilation #3758

Closed

Conversation

cwong-ocaml
Copy link
Collaborator

As per discussion in #3182.

It's a little sloppy, but I think it works.

cwong-ocaml and others added 4 commits September 3, 2020 12:29
Signed-off-by: Cameron Wong <cwong@janestreet.com>
Signed-off-by: Cameron Wong <cwong@janestreet.com>
Signed-off-by: Cameron Wong <cwong@janestreet.com>
@cwong-ocaml
Copy link
Collaborator Author

Gah, just kidding, Compilation_context.modes doesn't work the same way I thought it did. Would appreciate some advice.

@cwong-ocaml
Copy link
Collaborator Author

cwong-ocaml commented Sep 8, 2020

Looks like this feature is going to take a bit longer than I expected -- right now, cmti files are generated with the same rules as the cmi files, but cmi files are always in the byte build folder. This is causing problems for merlin rules that expects to find cmti files in the same folder as cmt files.

The three paths forward that I see are as follows:

  • Adjust the merlin rules to find the cmti files in the byte build folder
    • I don't like this solution -- as far as I can tell, currently the merlin rules make no distinction between cmt and cmti files, so this would be a somewhat involved process for a relatively ad-hoc solution.
  • Move .cmti files to native after generating them with the existing rules
    • This solution is definitely the hackiest one, but probably the quickest and least immediately bug-prone
  • Change the rules for cmi generation to be generated in the same place as the cmt files (either byte or native) instead of hardcoding it to bytecode mode
    • I think this makes the most sense, and is what I'll be working on moving forward

@rgrinberg
Copy link
Member

I'd like to revisit the discussion of this issue before more effort is spent on this. The author never clarified why he disabled bytecode builds in the first place. If it was for a reason like "optimizing" the build, then I don't think this issue is worth addressing.

@cwong-ocaml
Copy link
Collaborator Author

Understood; I'll shelve this for the time being. Perhaps @jeremiedimino will have thoughts once he returns.

@rgrinberg
Copy link
Member

Sorry about not warning you earlier. I had this issue assigned earlier and arrived at similar hacks as in this PR. In the end, I struggled to justify adding so much crud to address address a non-issue for the vast majority of users. Not to mention that adding (modes byte) is an incredibly simple workaround. I'm unaware of any situations where the bytecode compiler isn't available.

@cwong-ocaml cwong-ocaml requested review from a user and nojb September 8, 2020 19:46
@cwong-ocaml
Copy link
Collaborator Author

oof, misclicked the review request button, sorry!

@cwong-ocaml cwong-ocaml removed the request for review from nojb September 8, 2020 19:47
@jberdine
Copy link
Contributor

jberdine commented Sep 8, 2020

Note that for the related issue #3467, the root motivation is not about the bytecode compiler not being available. There is a goal to make every warning/error from the compiler appear exactly once. Enabling both byte and native modes leads to duplicated warnings and errors. Turning off warnings in ocamlc_flags doesn't work since .mli files are only compiled with ocamlc. So for that particular case, a way to pass different flags to ocamlc when compiling .ml and .mli would be a workaround.

(Also for the record, without thorough benchmarks I don't think that just dismissing the build time of bytecode as irrelevant is necessarily justified. Compiling to bytecode still has to perform type inference, which for some code is not trivial.)

@rgrinberg
Copy link
Member

Thanks for helping me understand the issue @jberdine.

There is a goal to make every warning/error from the compiler appear exactly once. Enabling both byte and native modes leads to duplicated warnings and errors.

I think that disabling bytecode builds to solve this problem is a bad workaround. Byte code builds are incredibly useful for many reasons (debug, toplevel, jsoo) and it's not practical to disable them for most of us.

(Also for the record, without thorough benchmarks I don't think that just dismissing the build time of bytecode as irrelevant is necessarily justified. Compiling to bytecode still has to perform type inference, which for some code is not trivial.)

I'm dismissing this workaround rather than the issue. If type checking is so expensive, you don't have to use @all. Individual targets should be much better in such builds.

By the way, both of these problems can be solved by improving the compiler to support a separate type checking phase that produces cmi, cmt, etc. This solution would solve the double warnings and improve build times. Would be great to explore this solution rather than pile on band aids in dune.

@jberdine
Copy link
Contributor

jberdine commented Sep 8, 2020

I agree that disabling bytecode compilation is a bit of a crude club-style workaround, and it may not be worth lots of hacks in dune, but in that case I think that the modes config needs to be documented differently, as the current behavior seems to be clearly in conflict with what the docs say.

I had the understanding from long ago, probably now-stale information, that at least part of the problem is that there are multiple ways to generate e.g. .cmi from .mli, either using ocamlc or ocamlopt. My understanding is that because of reasons (to do with build systems liking obviously commuting graphs or something), dune chooses to only use one of those possible paths from .mli to .cmi, leading to e.g. invocation of ocamlc even when producing only native code. Finding a way around that sort of situation seems like it might be solvable in the land of the build system without resorting to hacky workarounds. Maybe. I don't know if the situation with .cmt files is analogous or not.

But I agree that being able to stop/continue after typing would be best. Implementing that feels like it could share a lot with the ocamlfdo integration enabled by the fdo context optional field.

@cwong-ocaml
Copy link
Collaborator Author

After some further discussion, I think we've decided that, while everyone's concerns are valid, this particular approach is probably not the correct one for dune. Moving forward, we are planning to submit a compiler patch to be able to resume compilation from .cmt files, and then adjust the rules to first build in one mode, then build from the .cmts in the other when asked.

Pending performance testing, this should resolve many issues much more nicely than this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants