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

JBuilder sometimes does not recompile executables #237

Closed
smondet opened this issue Aug 25, 2017 · 11 comments
Closed

JBuilder sometimes does not recompile executables #237

smondet opened this issue Aug 25, 2017 · 11 comments

Comments

@smondet
Copy link

smondet commented Aug 25, 2017

I have a very simple one-ocaml-file → one-executable project

(jbuild_version 1)

(executable
 ((name vidimetro)
  (libraries (lablgl.glut nonstd sosa cmdliner misuja))))

(install ((section bin) (files ((vidimetro.exe as vidimetro)))))

I've noticed that some small but functional changes to vidimetro.ml do not trigger relinking of the executable:

 $ jbuilder --version
1.0+beta12
 $ jbuilder build --dev @install
    ocamldep vidimetro.depends.ocamldep-output
      ocamlc vidimetro.{cmi,cmo,cmt}
    ocamlopt vidimetro.{cmx,o}

it does not rebuild vidimetro.exe.

If I clean I get the right .exe:

 $ jbuilder clean
 $ jbuilder build --dev @install
    ocamldep vidimetro.depends.ocamldep-output
      ocamlc vidimetro.{cmi,cmo,cmt}
    ocamlopt vidimetro.{cmx,o}
    ocamlopt vidimetro.exe

(I doubt that it is a file time-stamps problem, I'm on a standard ext4 file-system)

(the code is in this branch: https://gitlab.com/smondet/vidimetro/commits/packaging-improvements)

PS: maybe (?) we had similar issues in solvuu-build, need to depend on .cmx and .o files
solvuu/solvuu-build#65

@jberdine
Copy link
Contributor

I have not had time to construct a small reproduction case, but FWIW I have experienced this same issue with beta12, beta11 is ok.

@rgrinberg
Copy link
Member

Note that this is unrelated to timestamps as the staleness mechanism uses checksums in beta12. I'll investigate but we'll likely need @diml here.

@dra27
Copy link
Member

dra27 commented Aug 26, 2017

It is in fact related to timestamps, but subtly. An executable only depends on the .cmx files, which was fine when we used timestamps but is not fine now that we compare contents, because there are many changes which do not affect .cmx files.

@dra27
Copy link
Member

dra27 commented Aug 26, 2017

This should be fixed in #238 - @smondet, I've tested it with the repo here, @jberdine - if you're able to test it (opam pin add jbuilder https://github.com/dra27/jbuilder.git#fix-exe-compilation), that'd also be handy, thanks!

@smondet
Copy link
Author

smondet commented Aug 26, 2017

@dra27 I just tried opam pin add jbuilder https://github.com/dra27/jbuilder.git#fix-exe-compilation, it works 👍

About the question in #238, yes I think cmxa and cmxs files should also depend on the .o (better safe than sorry after all :) ).

@dra27
Copy link
Member

dra27 commented Aug 26, 2017

@smondet - actually, I was meaning whether executables should depend on the .a file in addition to the .cmxa (I think, looking at the OCaml sources, that the same thing can happen - .cmxa identical, but .a file changed) but you are also right that the compilation of the libraries themselves should do the same thing!

@jberdine
Copy link
Contributor

jberdine commented Aug 26, 2017 via email

@copy
Copy link

copy commented Aug 29, 2017

In case someone runs into the same issue as I did: ppx_expect failures of the kind File … changed, you need rebuild inline_test_runner to be able to run expect tests are also fixed by this.

jvillard referenced this issue in facebook/infer Aug 30, 2017
Summary:
Use jbuilder to build infer instead of ocamlbuild. This is mainly to get faster builds:

```
times in 10ms, ±differences measured in speedups, 4 cores
|                                   | ocb total |   jb | ±total | ocb user |   jb | ±user | ocb cpu |  jb | ±cpu | ocb sys |   jb | ±sys |
|-----------------------------------+-----------+------+--------+----------+------+-------+---------+-----+------+---------+------+------|
| byte from scratch                 |      6428 | 2456 |   2.62 |     7743 | 6662 |  1.16 |     138 | 331 | 2.40 |    1184 | 1477 | 0.80 |
| native from scratch               |      9841 | 4289 |   2.29 |     9530 | 8834 |  1.08 |     110 | 245 | 2.23 |    1373 | 1712 | 0.80 |
| byte after native                 |     29578 | 1602 |  18.46 |     4514 | 4640 |  0.97 |     170 | 325 | 1.91 |     543 |  576 | 0.94 |
| change infer.ml byte              |       344 |  282 |   1.22 |      292 |  215 |  1.36 |      96 |  99 | 1.03 |     040 |  066 | 0.61 |
| change infer.ml native            |       837 |  223 |   3.75 |      789 |  174 |  4.53 |      98 |  99 | 1.01 |     036 |   47 | 0.77 |
| change Config.ml byte             |       451 |  339 |   1.33 |      382 |  336 |  1.14 |      97 | 122 | 1.26 |     056 |   80 | 0.70 |
| change Config.ml native           |      4024 | 1760 |   2.29 |     4585 | 4225 |  1.09 |     127 | 276 | 2.17 |     559 |  644 | 0.87 |
| change cFrontend_config.ml byte   |       348 |  643 |   0.54 |      297 |  330 |  0.90 |      96 |  67 | 0.70 |     038 |  102 | 0.37 |
| change cFrontend_config.ml native |      1480 |  584 |   2.53 |     1435 |  906 |  1.58 |     106 | 185 | 1.75 |     136 |  178 | 0.76 |
#+TBLFM: $4=$2/$3;f2::$7=$5/$6;f2::$10=$9/$8;f2::$13=$11/$12;f2

50 cores
|                     | ocb total |   jb | ±total | ocb user |   jb | ±user | ocb cpu | jb | ±cpu | ocb sys |   jb | ±sys |
|---------------------+-----------+------+--------+----------+------+-------+---------+----+------+---------+------+------|
| byte from scratch   |      9114 | 2061 |   4.42 |     9334 | 5133 |  1.82 |         |    | 0/0  |    2566 | 1726 | 1.49 |
| native from scratch |     13481 | 3967 |   3.40 |    12291 | 7608 |  1.62 |         |    | 0/0  |    3003 | 2100 | 1.43 |
| byte after native   |      3467 | 1476 |   2.35 |     5067 | 3912 |  1.30 |         |    | 0/0  |     971 |  801 | 1.21 |
#+TBLFM: $4=$2/$3;f2::$7=$5/$6;f2::$10=$9/$8;f2::$13=$11/$12;f2
```

Menu:

1. Write a jbuild file, autogenerated from jbuild.in because we need to fill in
  some information at build-time (really, at configure time, but TODO), such as
  whether or not clang is enabled.
2. Nuke lots of stuff from infer/src/Makefile that is now in the jbuild file
3. The jbuild file lives in infer/src/ so it can see all the sources. If we put it somewhere else, eg, infer/, then `jbuilder` scans too many files (all irrelevant) and takes 2.5s to start instead of .8s. Adding irrelevant directories to jbuild-ignore does not help.
4. jbuilder does not support subdirectories, so resort to listing all the
  source files in the generated jbuild (only source directories need to be
  manually listed in jbuild.in though). Still, the generated .merlin is wrong
  and makes merlin find source files in _build, so manually tune it to get
  good merlin support. We also lose some of merlin for unit tests as it
  cannot see their build artefacts anymore.
5. checkCopyright gets its own jbuild because it's standalone. Also, remove
  some deprecation warnings in checkCopyright due to the new version of Core from
  a while ago.
6. Drop less-used Makefile features (they had regressed anyway) such as
  building individual modules. Also, building mod_dep.pdf now takes all the
  source files available so they better build (before, it would only take the
  source files from the config, eg with or without clang) (that's pretty minor).
7. The toplevel is now built as a custom toplevel because that was easier. It
  should soon be even easier: ocaml/dune#210
8. Move BUILTINS.mli to BUILTINS.ml because jbuilder is not happy about
  interface files without implementations.

In particular, I did not try to migrate too much of the Makefile logic to jbuilder,
more can be done in the future.

Reviewed By: jberdine

Differential Revision: D5573661

fbshipit-source-id: 4ca6d8f
@edwintorok
Copy link
Contributor

I've just run into this bug when editing a constant in a file, it can be reproduced with this testcase: https://gist.github.com/edwintorok/a5d0492467f80fcbb226635f1f63da3c.
I confirm that the fix from @dra27 works.

@smondet
Copy link
Author

smondet commented Sep 6, 2017

@dra27 @rgrinberg was the issue also fixed in the case of the .cmxas and .as?

@dra27
Copy link
Member

dra27 commented Sep 6, 2017

@smondet - within a jbuilder project, it's not necessary for .cmxa to depend on .a, since the .cmxa will have been rebuilt if any of the modules in it were rebuilt (the .cmxa does now depend on the .o files). So the executable will always be relinked if the underlying the .a file changed because we can guarantee that so did the .cmxa.

I haven't convinced myself that it's impossible for packages coming from findlib (i.e. opam) to hit this, though.

dra27 added a commit to dra27/dune that referenced this issue Sep 6, 2017
Used while fixing ocaml#237 to work out why executables weren't being
relinked. Could be worth spinning off into a proper debug trace...
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

No branches or pull requests

6 participants