Fix bigarray config (#5494) #5526

moyodiallo · 2022-03-21T11:26:53Z

This is a tentative fix for #5494, bigarray issue on OCaml 5.00
I explored 2 solutions and I chose the second one.

It's to avoid generating bigarray in Meta files like mentioned in the issue Successfully sun-setting the bigarray package #5494. But with the generation of Meta files, bigarray has to be resolved before and that raises an exception.
The second is to avoid resolving bigarray, that also doesn't generate it in the Meta files.

I'm not sure this is the right way to go about this problem, let me know if anyone thought of a better solution and I can make adjustments to this PR.

cc @kit-ty-kate

dra27 · 2022-03-26T08:26:49Z

Thanks for this! Comparing opam-repository for current trunk OCaml (with opam-alpha-repository enabled), this unblocks 56 packages and reveals 7 new failing packages - several of which look they have pertinent errors.

src/dune_rules/lib.ml

nojb

What is the status of this? I see there is some discussion around the warning... I would suggest to keep the fix minimal by removing the warning and simply focusing on making bigarray a no-op in OCaml 5 and later. The warning can be added later if necessary/wanted.

dra27 · 2022-04-19T17:49:50Z

As it happens, I started reviewing and poking this today, too, @nojb! I think the other two cases need (re_export and select) and then it's good to go. It can indeed be merged without the warning.

FWIW, it should be possible to extend this behaviour back to 4.08 - IIRC since 4.08 it has not been necessary ever to use the legacy bigarray library, but it would be better to do that in a separate PR (if at all), as well.

Implementing "Select" rule and "Re_export" witch is the same as "Direct". ocaml#5526 (comment) is a good report for this fix. Signed-off-by: Alpha DIALLO <[email protected]>

moyodiallo · 2022-04-21T08:17:25Z

I think it's ready for last review then to be merged.

test/blackbox-tests/test-cases/bigarray-select.t

moyodiallo · 2022-04-28T17:11:44Z

+ let required = Lib_name.Set.filter ~f:(fun lib -> not(String.equal (Lib_name.to_string lib) "bigarray")) required in

Hi @dra27, I noticed we could end-up require and forbidden empty so that switch quietly in the case "(_ -> ...)".

emillon

OK, let's get that one past the finish line. I'd like to make sure we can keep this logic not to spread around and we should be able to fix that for the next version of dune.

src/dune_rules/lib.ml

test/blackbox-tests/test-cases/bigarray-skip/run.t

Implementing "Select" rule and "Re_export" witch is the same as "Direct". ocaml#5526 (comment) is a good report for this fix. Signed-off-by: Alpha DIALLO <[email protected]>

Implementing "Select" rule and "Re_export" witch is the same as "Direct". ocaml#5526 (comment) is a good report for this fix. Signed-off-by: Alpha DIALLO <[email protected]> Signed-off-by: Etienne Millon <[email protected]>

emillon · 2022-05-24T14:39:48Z

I pulled the code in a way that adds a new "rewrite" step before the rest of the processing. This ensures that this PR is a no-op for OCaml 4.x. For the part that rewrites the lib dep, we create a new lib dep that is interpreted as before. This is done with the same rules we would use if we were upgrading a project to OCaml 5+.
The rules I used based on @moyodiallo's code are the following. In technical terms "rewriting" a list is a filter_map operation since we can drop or modify items.

for direct dependendencies and reexports: drop them if the library is bigarray, or copy as is otherwise
for each select form, rewrite into a select form. the list of choices is rewritten using the following rules:
- if a choice contains !bigarray (negative / "forbidden"), remove it completely.
- remove bigarray (positive / "required") from the list.
- rationale: evaluating choices is like evaluating boolean formulas and we're making a partial evaluation. !bigarray is False, so !bigarray & ... evaluates to False. Since choices are evaluated sequentially, this is equivalent to removing the choice. And bigarray is True, which is neutral for & so it's equivalent to removing it from the choice.

There's a mildly interesting corner case which happens when no choices are left after rewriting. This happens with (select a from (!bigarray -> b)). The docs say that it's an error when no choice matches, so we should rewrite to a form that creates an error. We should need to be careful the select form with no choices if there's no concrete syntax for it, but fortunately (and a bit surprisingly), the following is already accepted and supported: (select a from). So there's no special case to handle.

Implementing "Select" rule and "Re_export" witch is the same as "Direct". ocaml#5526 (comment) is a good report for this fix. Signed-off-by: Alpha DIALLO <[email protected]> Signed-off-by: Etienne Millon <[email protected]>

The bigarray library has already inculded in the Standard Library since OCaml 4.07, the library has been removed in OCaml 5.00 and this fix avoid an Error durig resolving the library by just emitting a Warning. Signed-off-by: Alpha DIALLO <[email protected]> Signed-off-by: Etienne Millon <[email protected]>

The skip of bigarray remains Signed-off-by: Alpha DIALLO <[email protected]> Signed-off-by: Etienne Millon <[email protected]>

Implementing "Select" rule and "Re_export" witch is the same as "Direct". ocaml#5526 (comment) is a good report for this fix. Signed-off-by: Alpha DIALLO <[email protected]> Signed-off-by: Etienne Millon <[email protected]>

A new test case Select rule with bigarray Signed-off-by: Alpha DIALLO <[email protected]> Signed-off-by: Etienne Millon <[email protected]>

"Select" rule prevent the case when we endup on require and forbiden empty. Skipping bigarray could result having require and forbiden empty that case is the same as no literals "(_ -> dummy)". Some test-cases for "Direct", "Re-export" and "Select" Signed-off-by: Alpha DIALLO <[email protected]> Signed-off-by: Etienne Millon <[email protected]>

Signed-off-by: Etienne Millon <[email protected]>

Signed-off-by: David Allsopp <[email protected]>

dra27 · 2022-06-07T14:33:20Z

I rebased and propose b2958db to update the test case - this runs three different programs which exercise the three cases in the code. The test is expected to run in the same way on any OCaml version - i.e. when Dune starts testing OCaml 5.0 in CI then that test should remain unchanged (I ran it locally with OCaml 5, of course).

nojb · 2022-06-07T15:12:11Z

I rebased and propose b2958db to update the test case - this runs three different programs which exercise the three cases in the code. The test is expected to run in the same way on any OCaml version - i.e. when Dune starts testing OCaml 5.0 in CI then that test should remain unchanged (I ran it locally with OCaml 5, of course).

In ocaml/ocaml#11007 (comment) I suggested shipping META.bigarray in the compiler (I had forgotten about this PR), which seems to be at odds with the approach taken here for OCaml >= 5 (right?). To be honest, at this point I can't tell anymore which one is the "good" approach :) Anyway, it would be great if someone who has clarity about the issue were to add a comment (either for or against) in the discussion I linked. Thanks!

dra27 · 2022-06-07T16:39:37Z

I'm just paging in the OCaml PR, but we definitely shouldn't have a META file bigarray, because there is no bigarray package anymore (this might get extended to bytes, but that's a separate question). (#5494, from a while ago now, has the fuller detail!)

nojb · 2022-06-07T17:02:27Z

I'm just paging in the OCaml PR, but we definitely shouldn't have a META file bigarray, because there is no bigarray package anymore (this might get extended to bytes, but that's a separate question). (#5494, from a while ago now, has the fuller detail!)

Right, I'll make a note upstream, thanks for the clarifying.

moyodiallo · 2022-06-09T18:17:49Z

I rebased and propose b2958db to update the test case - this runs three different programs which exercise the three cases in the code. The test is expected to run in the same way on any OCaml version - i.e. when Dune starts testing OCaml 5.0 in CI then that test should remain unchanged (I ran it locally with OCaml 5, of course).

Could it be necessary to test which file is emitted from the case 3 (select) according to our fix, to give this test more sense.

dra27 · 2022-06-09T20:35:11Z

Could it be necessary to test which file is emitted from the case 3 (select) according to our fix, to give this test more sense.

It does - each of the files prints a different message, so if the wrong file is selected the cram test will fail because of different output.

emillon · 2022-06-13T10:34:00Z

Thanks! I rebased and tested that on the 5.0 branch.

Signed-off-by: Etienne Millon <[email protected]>

@rgrinberg

…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.3.0) CHANGES: - Sandbox preprocessing, lint, and dialect rules by default. All these rules now require precise dependency specifications (ocaml/dune#5807, @rgrinberg) - Allow list expansion in the `pps` specification for preprocessing (ocaml/dune#5820, @Firobe) - Add warnings 67-69 to dune's default set of warnings. These are warnings of the form "unused X.." (ocaml/dune#5844, @rgrinbreg) - Introduce project "composition" for coq theories. Coq theories in separate projects can now refer to each other when in the same workspace (ocaml/dune#5784, @Alitzer, @rgrinberg) - Fix hint message for ``data_only_dirs`` that wrongly mentions the unknown constructor ``data_only`` (ocaml/dune#5803, @lambdaxdotx) - Fix creating sandbox directory trees by getting rid of buggy memoization (@5794, @rgrinberg, @snowleopard) - Handle directory dependencies in sandboxed rules. Previously, the parents of these directory dependencies weren't created. (ocaml/dune#5754, @rgrinberg) - Set the exit code to 130 when dune is terminated with a signal (ocaml/dune#5769, fixes ocaml/dune#5757) - Support new locations of unix, str, dynlink in OCaml >= 5.0 (ocaml/dune#5582, @dra27) - The ``coq.theory`` stanza now produces rules for running ``coqdoc``. Given a theory named ``mytheory``, the directory targets ``mytheory.html/`` and ``mytheory.tex/`` or additionally the aliases `@doc` and `@doc-latex` will build the HTML and LaTeX documentation repsectively. (ocaml/dune#5695, fixes ocaml/dune#3760, @Alizter) - Coq theories marked as `(boot)` cannot depend on other theories (ocaml/dune#5867, @ejgallego) - Ignore `bigarray` in `(libraries)` with OCaml >= 5.0. (ocaml/dune#5526, fixes ocaml/dune#5494, @moyodiallo) - Start with :standard when building the ctypes generated foreign stubs so that we include important compiler flags, such as -fPIC (ocaml/dune#5816, fixes ocaml/dune#5809).

rgrinberg · 2023-10-09T23:33:57Z

src/dune_rules/lib.ml

+      in
+      if bigarray_in_std_libraries then deps
+      else
+        let bigarray = Lib_name.of_string "bigarray" in


This change is incorrect and introduces a bug that forbids local libraries named bigarray from being used on OCaml < 5.0.

True, I don't know how we can test if there a local library called bigarray.

that has been fixed in #8902 .

kit-ty-kate mentioned this pull request Mar 31, 2022

Add support for OCaml 5.00 (again) ocsigen/lwt#933

Closed

emillon reviewed Mar 31, 2022

View reviewed changes

src/dune_rules/lib.ml Outdated Show resolved Hide resolved

src/dune_rules/lib.ml Outdated Show resolved Hide resolved

nojb reviewed Apr 19, 2022

View reviewed changes

moyodiallo force-pushed the fix-bigarray branch from 6ecac5e to 6c8d122 Compare April 19, 2022 17:09

kit-ty-kate mentioned this pull request Apr 20, 2022

Require bigarray-compat to enable lwt.unix under OCaml 5.00 ocsigen/lwt#930

Closed

kit-ty-kate reviewed Apr 26, 2022

View reviewed changes

test/blackbox-tests/test-cases/bigarray-select.t Outdated Show resolved Hide resolved

moyodiallo force-pushed the fix-bigarray branch from 5bfff0b to e8d9458 Compare April 28, 2022 17:06

emillon reviewed May 23, 2022

View reviewed changes

src/dune_rules/lib.ml Outdated Show resolved Hide resolved

src/dune_rules/lib.ml Outdated Show resolved Hide resolved

test/blackbox-tests/test-cases/bigarray-skip/run.t Outdated Show resolved Hide resolved

emillon mentioned this pull request May 23, 2022

refactor: Resolve_names : extract a builder module #5762

Merged

emillon linked an issue May 23, 2022 that may be closed by this pull request

Successfully sun-setting the bigarray package #5494

Closed

emillon self-assigned this May 23, 2022

emillon force-pushed the fix-bigarray branch from e8d9458 to cc184d8 Compare May 24, 2022 10:19

emillon force-pushed the fix-bigarray branch from 9650e70 to 0544bf4 Compare May 24, 2022 13:30

emillon force-pushed the fix-bigarray branch from 0544bf4 to 1055fa9 Compare May 24, 2022 14:40

emillon force-pushed the fix-bigarray branch from 1055fa9 to 4f2c9b8 Compare May 25, 2022 08:44

emillon force-pushed the fix-bigarray branch from 4f2c9b8 to 03f0a04 Compare May 25, 2022 09:20

emillon force-pushed the fix-bigarray branch from 03f0a04 to 899d9e3 Compare May 30, 2022 13:35

moyodiallo and others added 8 commits June 7, 2022 15:31

keep it simple by removing the warning message

b1f451d

The skip of bigarray remains Signed-off-by: Alpha DIALLO <[email protected]> Signed-off-by: Etienne Millon <[email protected]>

Fix bigarray config ocaml#5494

047c6ed

Implementing "Select" rule and "Re_export" witch is the same as "Direct". ocaml#5526 (comment) is a good report for this fix. Signed-off-by: Alpha DIALLO <[email protected]> Signed-off-by: Etienne Millon <[email protected]>

Fix bigarray config ocaml#5494

4b5f43c

A new test case Select rule with bigarray Signed-off-by: Alpha DIALLO <[email protected]> Signed-off-by: Etienne Millon <[email protected]>

Turn filtering into a separate step

cb2c7d2

Signed-off-by: Etienne Millon <[email protected]>

Fix formatting

e90db67

Signed-off-by: Etienne Millon <[email protected]>

Simplify rewrite

0019ae4

Signed-off-by: Etienne Millon <[email protected]>

Update test case

b2958db

Signed-off-by: David Allsopp <[email protected]>

dra27 force-pushed the fix-bigarray branch from 899d9e3 to b2958db Compare June 7, 2022 14:32

emillon added this to the 3.3.0 milestone Jun 7, 2022

moyodiallo force-pushed the fix-bigarray branch from b871ec7 to b2958db Compare June 8, 2022 19:17

dra27 mentioned this pull request Jun 12, 2022

Do not depend on ocamlfind in base-bytes ocaml/opam-repository#21333

Closed

Merge branch 'main' into fix-bigarray

1ec12f0

emillon merged commit 618bc8f into ocaml:main Jun 13, 2022

dra27 mentioned this pull request Jun 13, 2022

Fix bigarray support for OCaml 5.x [2.9] #5872

Merged

emillon added a commit to emillon/dune that referenced this pull request Jun 13, 2022

Add changelog entry for ocaml#5526

349213a

Signed-off-by: Etienne Millon <[email protected]>

emillon added a commit that referenced this pull request Jun 13, 2022

Add changelog entry for #5526 (#5873)

b8e0fe7

Signed-off-by: Etienne Millon <[email protected]>

emillon mentioned this pull request Jun 17, 2022

[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, dune-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.3.0) ocaml/opam-repository#21555

Merged

moyodiallo mentioned this pull request Jun 22, 2022

Adding ocaml-bench/sandmark dependency packages ocurrent/ocaml-multicore-ci#21

Merged

rgrinberg reviewed Oct 9, 2023

View reviewed changes

rgrinberg mentioned this pull request Oct 9, 2023

Local libraries named bigarray are incorrectly removed #8901

Closed

moyodiallo deleted the fix-bigarray branch October 11, 2023 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bigarray config (#5494) #5526

Fix bigarray config (#5494) #5526

moyodiallo commented Mar 21, 2022

dra27 commented Mar 26, 2022

nojb left a comment

dra27 commented Apr 19, 2022

moyodiallo commented Apr 21, 2022

moyodiallo commented Apr 28, 2022 •

edited

Loading

emillon left a comment

emillon commented May 24, 2022

dra27 commented Jun 7, 2022

nojb commented Jun 7, 2022

dra27 commented Jun 7, 2022

nojb commented Jun 7, 2022

moyodiallo commented Jun 9, 2022

dra27 commented Jun 9, 2022

emillon commented Jun 13, 2022

rgrinberg Oct 9, 2023

moyodiallo Oct 10, 2023

emillon Oct 10, 2023

Fix bigarray config (#5494) #5526

Fix bigarray config (#5494) #5526

Conversation

moyodiallo commented Mar 21, 2022

dra27 commented Mar 26, 2022

nojb left a comment

Choose a reason for hiding this comment

dra27 commented Apr 19, 2022

moyodiallo commented Apr 21, 2022

moyodiallo commented Apr 28, 2022 • edited Loading

emillon left a comment

Choose a reason for hiding this comment

emillon commented May 24, 2022

dra27 commented Jun 7, 2022

nojb commented Jun 7, 2022

dra27 commented Jun 7, 2022

nojb commented Jun 7, 2022

moyodiallo commented Jun 9, 2022

dra27 commented Jun 9, 2022

emillon commented Jun 13, 2022

rgrinberg Oct 9, 2023

Choose a reason for hiding this comment

moyodiallo Oct 10, 2023

Choose a reason for hiding this comment

emillon Oct 10, 2023

Choose a reason for hiding this comment

moyodiallo commented Apr 28, 2022 •

edited

Loading