Skip to content

Use dune for installing the files in share#1902

Open
psafont wants to merge 2 commits into
herd:masterfrom
psafont:dev/pau/dunify
Open

Use dune for installing the files in share#1902
psafont wants to merge 2 commits into
herd:masterfrom
psafont:dev/pau/dunify

Conversation

@psafont

@psafont psafont commented Jul 7, 2026

Copy link
Copy Markdown

This allows to drop ad-hoc shell scripts that were used for installing the 'libdirs' of the tools.
Dune is able to do install entire source trees starting from version 3.11. The only difference I could see when dune installs is that the symlinks are converted to full files instead of being retained. This affects 4 .c files in litmus, as per the commit message.

@psafont psafont force-pushed the dev/pau/dunify branch 6 times, most recently from b3c9856 to b96e116 Compare July 7, 2026 14:26
cfg:
# Minimal versions: we want to test both ends of the supported space
- ocaml-compiler: "ocaml-base-compiler.4.14.0,dune.3.10.0,zarith.1.13,menhir.20231231,logs.0.7.0"
- ocaml-compiler: "ocaml-base-compiler.4.14.0,dune.3.12.1,zarith.1.13,menhir.20231231,logs.0.7.0"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The intent of this CI is to test with minimal versions. Is there a reason why this is not set to what version is promised to be sufficient?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sorry I didn't copy the comment from the commit to the first message here, there's no lower version available which is compatible as well:

On the CI workflows, the version cannot be set to 3.11.0 as that version was unreleased, 3.12.1 is the lowest version published above 3.11

@relokin relokin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this PR dumps the minimum version for dune, we need wider consensus. cc: @fsestini, @maranget, @HadrienRenaud.

@psafont you might also want to update INSTALL.md

@psafont

psafont commented Jul 7, 2026

Copy link
Copy Markdown
Author

you might also want to update INSTALL.md

Should I also bump dune's required version in asllib/dune-project and asllib/aslspec/dune-project?

@HadrienRenaud

Copy link
Copy Markdown
Collaborator

you might also want to update INSTALL.md

Should I also bump dune's required version in asllib/dune-project and asllib/aslspec/dune-project?

Difficult to say without testing, but I guess 2 minors version of dune is not a lot so I'd say go ahead.

@fsestini

fsestini commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks @psafont. I like the general direction of this PR, however I'd like to expand on some of its implications a bit more.

One thing that the dune-install.sh and dune-uninstall.sh script did was to completely wipe the libdir directories before (re)installing/uninstalling. See this line in dune-install.sh:

rm -rf "${to}" && mkdir -p "${to}" && ( cd "${from}" && cp -r . "${to}" )

dune-uninstall.sh did a similar thing.

I believe the point of this is to prevent stale files from past installations to survive into the respective libdirs after a reinstall of herdtools7.

As far as I can tell, dune install just overwrites files, and dune uninstall does not delete whole directories (unless they are empty), so the behaviour described above is not preserved by this PR. For example, we can see that extra files that were previously in the libdir (e.g. from a previous installation) survive after a (re)install:

$ touch ~/.local/share/herdtools7/herd/extra-file
$ make install PREFIX=/Users/filses01/.local
$ ll ~/.local/share/herdtools7/herd | grep extra-file
-rw-r--r--@  1 filses01  staff      0  8 Jul 10:10 extra-file

Note that I don't disagree with this new behaviour in principle. In fact, I think (un)install scripts in general should not remove entire user directories unconditionally, and the behaviour proposed by this PR follows standard practice AFAICT.

So, again, I'm in favour of this change in principle, and I expect it to be harmless in practice as well, since our tools mostly look up specific filenames from the libdirs rather than enumerating or bulk-loading everything in there. However, I think it's an aspect worth pointing out to ensure we are all aware of the implications and are on board with it.

@psafont psafont force-pushed the dev/pau/dunify branch 3 times, most recently from 0b3415b to 50a7ba1 Compare July 14, 2026 08:54
@psafont

psafont commented Jul 14, 2026

Copy link
Copy Markdown
Author

Just found a PR that's superseded by this one: #420

@psafont

psafont commented Jul 14, 2026

Copy link
Copy Markdown
Author

I've done a bit on investigation regarding the files types in the libdirs and how the tools use them to make sure that adding file doesn't interfere with how they work:

  • Herd
    • .cat files: all cat files are used by name, both from other cat files and in herd/model.ml
      • Tested with a random .cat file added and running litmus tests, no changes observed.
    • .cfg files: herd reads a single configuration file, which can be overriden by using -conf
  • jingle
    • .theme files are selected individually using the argument -theme
    • .call files are selected individually using the argument -call
    • .map files are selected individually using the argument -map
  • litmus
    • Some C files and headers are included, I added a header in the _aarch64 directory with a conflicting type definition for ins_t which defined byinstruction.h, and proceeded to generate the source code, the expected, correct definition what placed in the .c file, while the conflicting one was not used. It didn't have any effect.
    • .awk files individually copied by the objUtils.ml, by name
    • .cfg files are selected using the argument -mach, individually
  • tools
    • .cat file: the 2 existing ones don't seem to be references in tools/, uoaim had a parameter to load cat files with -cat

As far as I could see, changing the behaviour there doesn't change other that seeing a warning message when running make uninstall:

$ make uninstall PREFIX=/tmp/post
Error: Directory /tmp/post/share/herdtools7/litmus/_aarch64 is not empty,
cannot delete (ignoring).
Error: Directory /tmp/post/share/herdtools7/litmus is not empty, cannot
delete (ignoring).
Error: Directory /tmp/post/share/herdtools7/herd is not empty, cannot delete
(ignoring).

Pau Ruiz Safont added 2 commits July 14, 2026 13:55
Some of the tools need to have their libdirs copied to a subdirectory in
share, use dune to do this instead of using an ad-hoc script

Documentation on the share section for install and the globs install can
be found at https://dune.readthedocs.io/en/stable/reference/dune/install.html

This changes some symbolic links to be normal files, meaning they are
now duplicated when installing, this can be seen by diffing the
installed directories before and after the change:

  $ bash -c "diff <(tree -vi /tmp/pre) <(tree -vi /tmp/post)"
  1c1
  < /tmp/pre
  ---
  > /tmp/post
  3133c3133
  < timebase.c -> timebase64.c
  ---
  > timebase.c
  3156,3157c3156,3157
  < _cache.h -> ../_x86/_cache.h
  < barrier.c -> ../_x86/barrier.c
  ---
  > _cache.h
  > barrier.c
  3164,3165c3164,3165
  < mbar.c -> ../_x86/mbar.c
  < timebase.c -> ../_x86/timebase.c
  ---
  > mbar.c
  > timebase.c

On the CI workflows, the version cannot be set to 3.11.0 as that version
was unreleased, 3.12.1 is the lowest version published above 3.11
This keeps in line with herdtools7's dune lang requirement

@fsestini fsestini left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for looking into this @psafont . This LGTM.

@relokin relokin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks Pau. If you rebase on top of master, I will merge this PR.

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.

4 participants