`(no_dynlink)`: do not build `.cmxs` #11176

nojb · 2024-12-02T10:16:59Z

What it says on the tin: when the library declares (no_dynlink), then the .cmxs should not be built. On large codebases, this can save considerable time (especially on Windows).

nojb · 2024-12-02T10:24:38Z

@rgrinberg: do you think this change would need to be versioned?

rgrinberg · 2024-12-02T22:42:10Z

test/blackbox-tests/test-cases/no_dynlink.t

+
+  $ touch a.ml
+
+  $ dune build --display short


Could you replace the --display short with just an explicit target such as
mylib.cmxs?

This extra output makes the tests fragile and hard to understand

Fixed, thanks.

rgrinberg · 2024-12-02T22:42:30Z

test/blackbox-tests/test-cases/no_dynlink.t

@@ -0,0 +1,37 @@
+  $ cat >dune-project <<EOF


Can you add a few sentences here to describe the test?

Fixed, thanks.

rgrinberg · 2024-12-02T22:43:50Z

It does sound fine to not version this. The old behavior was useless, so I
don't see much reason to preserve it.

Could you add a CHANGES entry?

nojb · 2024-12-03T08:18:17Z

Could you add a CHANGES entry?

It already has one :)

nojb · 2024-12-03T08:18:34Z

Thanks for the review @rgrinberg! I think all points are addressed.

anmonteiro · 2024-12-03T09:13:19Z

does this still build & install the .cmxs when building for -p .. and dune install?

nojb · 2024-12-03T09:23:35Z

does this still build & install the .cmxs when building for -p .. and dune install?

No. The .cmxs is not installed (even before this PR) if the library is declared with (no_dynlink). After this PR, the .cmxs is not even built by Dune (if (no_dynlink) is used, of course).

anmonteiro · 2024-12-03T09:37:03Z

I disagree: before this PR, a (public_name my_library) installs a my_lybrary.cmxs.

after this PR, a (public_name my_library) (no_dynlink) doesn't install my_library.cmxs.

it's technically a breaking change if these 2 points are true at the same time:

a library adds (no_dynlink) and releases to opam
another library was using Dynlink.loadfile "that_cmxs_file.cmxs".

note: no strong opinions whether you want to support this (arguable) edge case. i realize that this will help your builds internally and is indeed an edge case (and a new field) so i'm not opposed to it.

nojb · 2024-12-03T09:42:32Z

I disagree: before this PR, a (public_name my_library) installs a my_lybrary.cmxs.

I cannot reproduce this behaviour with Dune 3.16:

$ mkdir no_dynlink
$ cd no_dynlink
$ cat >dune-project <<EOF
> (lang dune 3.16)
> (package (name foo))
> EOF
$ cat >dune <<EOF
> (library (public_name foo) (no_dynlink))
> EOF
$ dune --version
3.16.0
$ dune build
$ find _build/install/default/
_build/install/default/
_build/install/default/lib
_build/install/default/lib/foo
_build/install/default/lib/foo/foo.cma
_build/install/default/lib/foo/META
_build/install/default/lib/foo/foo.cmt
_build/install/default/lib/foo/foo.a
_build/install/default/lib/foo/foo.cmi
_build/install/default/lib/foo/foo.ml
_build/install/default/lib/foo/dune-package
_build/install/default/lib/foo/foo.cmxa
_build/install/default/lib/foo/foo.cmx
$

nojb · 2024-12-03T10:07:15Z

@anmonteiro can you confirm whether you are observing the installation of .cmxs of (no_dynlink) libraries or not? And, if yes, can you provide precise reproduction instructions, so that I can investigate the issue? Thanks!

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Leonidas-from-XIV · 2024-12-03T13:49:19Z

@nojb I can confirm that Dune 3.16 has the behavior you describe (also I rebased the branch to get the fix for macOS CI failures in).

nojb · 2024-12-03T13:57:46Z

@nojb I can confirm that Dune 3.16 has the behavior you describe (also I rebased the branch to get the fix for macOS CI failures in).

Thanks!

anmonteiro · 2024-12-03T19:59:14Z

I understand now that dune 3.16 didn't install the cmxs in the first place. that means i'm wrong, please proceed.

anmonteiro · 2024-12-03T20:02:34Z

but something has changed between 3.16 and this PR where a .cmxs is installed with dune 3.17. I'm not sure if that's expected but i didn't find it in the changelog

rgrinberg · 2024-12-03T23:15:48Z

test/blackbox-tests/test-cases/no_dynlink.t

+
+  $ dune clean
+
+  $ dune build _build/default/mylib.cmxs


Could you add some tests for the .install file as well?

Good idea. Fixed, thanks.

rgrinberg · 2024-12-03T23:19:50Z

I disagree: before this PR, a (public_name my_library) installs a my_lybrary.cmxs.

after this PR, a (public_name my_library) (no_dynlink) doesn't install my_library.cmxs.

it's technically a breaking change if these 2 points are true at the same time:

a library adds (no_dynlink) and releases to opam

another library was using Dynlink.loadfile "that_cmxs_file.cmxs".

note: no strong opinions whether you want to support this (arguable) edge case. i realize that this will help your builds internally and is indeed an edge case (and a new field) so i'm not opposed to it.

To re-iterate nojb, I don't see how this PR changes installation. However, this PR is indeed breaking users that relied on building a cmxs but not installing it. I think such an edge case is useless enough that we can let it slide.

rgrinberg · 2024-12-03T23:20:42Z

I understand now that dune 3.16 didn't install the cmxs in the first place. that means i'm wrong, please proceed.

That is correct. With this PR, the cmxs won't even be built.

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

nojb · 2024-12-04T06:40:44Z

All issues having been addressed, planning to merge once CI passes.

* Do not build .cmxs when library is (no_dynlink) Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com> * Add test Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com> * Changes Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com> * Improve test Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com> * Add test Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com> --------- Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com> (cherry picked from commit 41212a5)

* `(no_dynlink)`: do not build `.cmxs` (#11176) * Do not build .cmxs when library is (no_dynlink) Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com> * Add test Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com> * Changes Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com> * Improve test Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com> * Add test Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com> --------- Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com> (cherry picked from commit 41212a5) * Fix ignored-dune-lock test on macos (#11179) The macos patch utility doesn't like it when a patch contains no hunks, so this change adds a hunk to the patch used in the ignored-dune-lock test. Signed-off-by: Stephen Sherratt <stephen@sherra.tt> --------- Signed-off-by: Stephen Sherratt <stephen@sherra.tt> Co-authored-by: Stephen Sherratt <stephen@sherra.tt>

@nojb

CHANGES: ### Fixed - When a library declares `(no_dynlink)`, then the `.cmxs` file for it is no longer built. (ocaml/dune#11176, @nojb) - Fix bug that could result in corrupted file copies by Dune, for example when using the `copy_files#` stanza or the `copy#` action. (@nojb, ocaml/dune#11194, fixes ocaml/dune#11193) - Remove useless error message when running `$ dune subst` in empty projects. (@rgrinberg, ocaml/dune#11204, fixes ocaml/dune#11200)

rgrinberg reviewed Dec 2, 2024

View reviewed changes

nojb force-pushed the no_dynlink_cmxs branch from df9080f to 8a6b87d Compare December 3, 2024 08:16

nojb added 4 commits December 3, 2024 14:48

Do not build .cmxs when library is (no_dynlink)

7524bfe

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Add test

a16b767

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Changes

769c00a

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Improve test

2efeefc

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Leonidas-from-XIV force-pushed the no_dynlink_cmxs branch from 8a6b87d to 2efeefc Compare December 3, 2024 13:48

rgrinberg approved these changes Dec 3, 2024

View reviewed changes

rgrinberg reviewed Dec 3, 2024

View reviewed changes

Add test

62fbe0b

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

nojb merged commit 41212a5 into ocaml:main Dec 4, 2024
27 of 28 checks passed

nojb deleted the no_dynlink_cmxs branch December 4, 2024 06:55

This was referenced Dec 9, 2024

[3.17] (no_dynlink): do not build .cmxs (#11176) #11189

Merged

Better doc for (no_dynlink) #11195

Merged

maiste mentioned this pull request Dec 16, 2024

3.17.1 release tracking #11215

Open

11 tasks

maiste mentioned this pull request Dec 17, 2024

[new release] dune (15 packages) (3.17.1) ocaml/opam-repository#27137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`(no_dynlink)`: do not build `.cmxs` #11176

`(no_dynlink)`: do not build `.cmxs` #11176

nojb commented Dec 2, 2024

nojb commented Dec 2, 2024

rgrinberg Dec 2, 2024

nojb Dec 3, 2024

rgrinberg Dec 2, 2024

nojb Dec 3, 2024

rgrinberg commented Dec 2, 2024

nojb commented Dec 3, 2024

nojb commented Dec 3, 2024

anmonteiro commented Dec 3, 2024

nojb commented Dec 3, 2024

anmonteiro commented Dec 3, 2024 •

edited

Loading

nojb commented Dec 3, 2024

nojb commented Dec 3, 2024

Leonidas-from-XIV commented Dec 3, 2024

nojb commented Dec 3, 2024

anmonteiro commented Dec 3, 2024

anmonteiro commented Dec 3, 2024

rgrinberg Dec 3, 2024

nojb Dec 4, 2024

rgrinberg commented Dec 3, 2024

rgrinberg commented Dec 3, 2024

nojb commented Dec 4, 2024

(no_dynlink): do not build .cmxs #11176

(no_dynlink): do not build .cmxs #11176

Conversation

nojb commented Dec 2, 2024

nojb commented Dec 2, 2024

rgrinberg Dec 2, 2024

Choose a reason for hiding this comment

nojb Dec 3, 2024

Choose a reason for hiding this comment

rgrinberg Dec 2, 2024

Choose a reason for hiding this comment

nojb Dec 3, 2024

Choose a reason for hiding this comment

rgrinberg commented Dec 2, 2024

nojb commented Dec 3, 2024

nojb commented Dec 3, 2024

anmonteiro commented Dec 3, 2024

nojb commented Dec 3, 2024

anmonteiro commented Dec 3, 2024 • edited Loading

nojb commented Dec 3, 2024

nojb commented Dec 3, 2024

Leonidas-from-XIV commented Dec 3, 2024

nojb commented Dec 3, 2024

anmonteiro commented Dec 3, 2024

anmonteiro commented Dec 3, 2024

rgrinberg Dec 3, 2024

Choose a reason for hiding this comment

nojb Dec 4, 2024

Choose a reason for hiding this comment

rgrinberg commented Dec 3, 2024

rgrinberg commented Dec 3, 2024

nojb commented Dec 4, 2024

`(no_dynlink)`: do not build `.cmxs` #11176

`(no_dynlink)`: do not build `.cmxs` #11176

anmonteiro commented Dec 3, 2024 •

edited

Loading