mirage / checkseum

MIT License
15 stars 13 forks source link

Opam installation fails with dune cache on #46

Closed kit-ty-kate closed 3 years ago

kit-ty-kate commented 4 years ago

Using the dune cache to speed up builds, I end up with this build failure with checkseum:

#=== ERROR while compiling checkseum.0.2.0 ====================================#
# context              2.1.0~alpha | linux/x86_64 | ocaml-base-compiler.4.10.0 | file:///home/opam/opam-repository
# path                 ~/.opam/4.10.0/.opam-switch/build/checkseum.0.2.0
# command              /bin/sh -exc echo "xen_linkopts = \"-l:laolao/xen/liblaolao_xen_stubs.a\"" >> _build/default/META.checkseum
# exit-code            2
# env-file             ~/.opam/log/checkseum-6-c5e94c.env
# output-file          ~/.opam/log/checkseum-6-c5e94c.out
### output ###
# /bin/sh: 1: cannot create _build/default/META.checkseum: Permission denied
# + echo xen_linkopts = "-l:laolao/xen/liblaolao_xen_stubs.a"

This is because files taken from the dune cache will not allow writes

mefyl commented 4 years ago

This is because dune now enforces that produced targets should never be modified by removing write permission to them. This both for theoretical zealous reasons (it makes no sense in the build theory to have two possible states for a built target) and for a very practical reason : the dune cache is coming very soon and shares such files with hardlinks, so in-place editing is proscribed.

The fix would be to either build META.checkseum in a single rule, including appending that last file, or produce a first file (say META.checkseum.gen) and build META.checkseum by appending said line.

One could also as a very quick workaround force dune <5.2 but that does not sound very user friendly.

dinosaure commented 4 years ago

The fix would be to either build META.checkseum in a single rule, including appending that last file, or produce a first file (say META.checkseum.gen) and build META.checkseum by appending said line.

I'm not sure that dune (with variants) allows us to modify META file. However, if it's possible to generate our own META file with a (rule ...), I will happy to try that.

mefyl commented 4 years ago

If you can manage, the rule version that produces the right file in one go is IMO the cleanest approach. Sorry for the inconvenience, I (we) really do think this setback is worth it in the long run.

rgrinberg commented 3 years ago

@dinosaure who reads the freestanding_linkopts variable from the META, and how?

avsm commented 3 years ago

The Mirage-3.9 mirage binary uses this as part of its compilation rules: https://github.com/mirage/mirage/blob/master/lib/mirage/mirage_impl_misc.ml#L63

Its to query all the unikernel link options required and then invoke those.

This problem will go away when mirage 4 is released (and we use dune workspaces), so perhaps we should punt on this until then and simply not enable the dune cache in the CI for now. (if you think a fix is complex, @rgrinberg )

rgrinberg commented 3 years ago

It would require a feature that has no immediate applications outside of this workaround. So it doesn't seem very attractive. I've glanced over that code, and it seems like this information could have been written to any other file. Perhaps that should be the workaround? Change the mirage tool and digestif to use their own file for communicating metadata rather than editing build files generated by dune.

ghost commented 3 years ago

Aren't META templates enough here? https://dune.readthedocs.io/en/latest/advanced-topics.html?highlight=template#meta-file-generation

rgrinberg commented 3 years ago

They are insufficient because dinosaure always wants to use virtual libraries. The two features are incompatible.

ghost commented 3 years ago

Ah yes, the problem is that if you have a custom META, then in the dune-package we say Use_meta. Is that right?

ghost commented 3 years ago

My impression is that in the long run, we will no longer need these xen_linkopts lines in META files, because of changes happening in Dune that will make things easier for mirage. @avsm is that your understanding as well?

If that's indeed the case, then we only need a workaround in the meantime. And like Rudi, I'm not excited about adding such a specific feature, especially if it's only needed for the short term.

Would it be enough to add this extra line via an extra build instruction in the opam file?

dinosaure commented 3 years ago

Would it be enough to add this extra line via an extra build instruction in the opam file?

This is what I currently do but it breaks some assumption about the cache (we should not set the META file as I do). I don't really ask something new about this situation which currently works and MirageOS 4 will fix that. I just would like to keep this situation as is until MirageOS 4 is out - and, then, delete all of these bad stuff.

ghost commented 3 years ago

Ah I see. So IIUC, you do: echo 'xen_linkopts = ...' >> xxx/META, and you get an error because the META file is read-only. What about changing this command to:

$ mv xxx/META xxx/META.old
$ { cat xxx/META.old; echo "xen_linkopts = ..." } > xxx/META
$ rm -f xxx/META.old

?

dinosaure commented 3 years ago

I do exactly that: https://github.com/mirage/checkseum/blob/a11c8b447c8cc45b952ef24113f39dd0f1cf095b/install/install.ml#L1-L24

Like, I update my right on the file - to be able to write on it - and append a line (xen_linkopts/freestanding_linkopts, same things). Currently, it does not disturb an usual installation and checkseum (& digestif) was released with this little script. The problem appears when we use dune-cache.

I never use it (/cc @mefyl) but it seems that dune-cache trust on the fact that after a dune build, nothing should change (and, by this way, it hashes files and it is able to cache them - I suppose). So, my little script breaks the rules and, as @kit-ty-kate said, we are not able to install checkseum if we use dune-cache.

Again, this situation: 1) it's specific to me 2) it's specific to dune-cache (opam and esy seems to work fine as far as I can tell) 3) it will be delete with MirageOS 4

So I think we should not waste our time on this specific problem and let it as is until the release of MirageOS 4 (it seems for me, the best option). But if you really want to fix that, I can improve on your way checkseum and digestif.

ghost commented 3 years ago

No particular opinion on whether it is worth doing something now. I will let @avsm comment on that.

To say two words about the technical reason: when someone uses the shared cache, files are shared on disk via hard links. In particular, inside the dune cache there is a dictionary mapping hash of file contents to files on disk. So if someone uses the shared cache and a compilation product is modified in place, then the shared cache gets corrupted. Indeed, you end up with an entry whose contents doesn't match the key.

To conclude this discussion: using the shared cache is not compatible with mutating compilation products in place. checkseum and digestif are currently mutating the META file generated by Dune in place and so are not compatible with the shared cache. With the next release of MirageOS, checkseum and digestif will no longer mutate META files generated by Dune in place and should then be compatible with the shared cache. The best way to make checkseum and digestif compatible with the shared cache before MirageOS 4 is to change their install scripts to not mutate the META file in place. For instance, by doing:

 let () = 
   let ic = open_in meta in
   let len = in_channel_length ic in
   let s = really_input_string ic len in
   close_in ic;
   Sys.remove meta;
   let oc = open_out meta in
   output_string oc s;
   output_char oc new_line;
   output_line oc freestanding;
   close_out oc

Since there is nothing more to do on the Dune side, I'm closing this issue.