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

regression on missing Coq #9818

Closed
vzaliva opened this issue Jan 24, 2024 · 5 comments · Fixed by #9845 or ocaml/opam-repository#25615
Closed

regression on missing Coq #9818

vzaliva opened this issue Jan 24, 2024 · 5 comments · Fixed by #9845 or ocaml/opam-repository#25615
Labels
Milestone

Comments

@vzaliva
Copy link

vzaliva commented Jan 24, 2024

Expected Behavior

Consider the following toy demo repository:

https://github.com/vzaliva/coq-dune-examlpe

In an opam switch without Coq installed, with dune 3.12.2 it builds without errors using following command:

dune build -p example-ocaml

(this is expected)

Actual Behavior

However, with dune 3.13.0 it gives build error:

$ dune build -p example-ocaml
Error: Couldn't find Coq standard library, and theory is not using (stdlib
no)

(this is a regression)

It is related to the following bug #8099
See the discussion there.

Specifications

$ dune --version
3.13.0
$ ocaml --version
The OCaml toplevel, version 4.13.1
$ lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 23.04
Release:	23.04
Codename:	lunar

Additional information

dune build -p example-ocaml --verbose
Shared cache: disabled
Shared cache location: /home/lord/.cache/dune/db
Workspace root: /home/lord/src/coq-dune-examlpe
Auto-detected concurrency: 12
Dune context:
 { name = "default"
 ; kind = "default"
 ; profile = Release
 ; merlin = true
 ; fdo_target_exe = None
 ; build_dir = In_build_dir "default"
 ; installed_env =
     map
       { "INSIDE_DUNE" : "/home/lord/src/coq-dune-examlpe/_build/default"
       ; "OCAML_COLOR" : "always"
       ; "OPAMCOLOR" : "always"
       }
 ; instrument_with = []
 }
Actual targets:
- recursive alias @install
Error: Couldn't find Coq standard library, and theory is not using (stdlib
no)
@emillon emillon added the coq label Jan 24, 2024
@ejgallego
Copy link
Collaborator

Thanks for the careful report @vzaliva , I think we should bisect to see what commit produces this error, but actually this may be an unintended result of a bug fix, that makes some important checks on rule creation.

We need to understand how the check is triggered, but in principle it should be possible and reasonably easy to make this check a bit more lazy so things work again.

@ejgallego
Copy link
Collaborator

Indeed, the error is trigged by Bootstrap.make. I'm am not sure how to make this check more lazy, as indeed it is needed to setup the Coq rules.

@rgrinberg , is it expected that the Coq rules are setup in this case?

@emillon
Copy link
Collaborator

emillon commented Jan 26, 2024

b6cf48c is the first bad commit (#9347)

@ejgallego
Copy link
Collaborator

uld bisect to see what commit produces this error, but actually this may be an unintended result of a bug fix, that makes s

Thanks @emillon , this is indeed the bug fix I was talking about. Before the bugfix, we did setup the rules, but we didn't error in this wrong configuration.

I'm not sure how to delay this check. One option is to remove the check, but that would introduce another kind of problems due to actual invalid configurations.

@rgrinberg
Copy link
Member

I was going to explain how to do avoid this issue in detail, but realized half way that prototype would be easier to understand. Basically, the issue is that doing User_error.raise or Resolve.Memo.read_memo are going to prevent rule loading for the entire directory. Depending on how fatal the error is, that's not what you want. The trick is to delay such errors until the rules are executed by plumbing them down to Action_builder.t using Resolve.Memo.t.

See #9845 for an example.

I'm not actually sure if it fixes the concrete error in this PR, but it should give you a sketch on how to go about this. Feel free to take over and adjust it to your tastes.

ejgallego pushed a commit that referenced this issue Feb 9, 2024
Fixes #9818

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: d2fe6b57-4512-4907-8577-43fdabcdcf8b -->
ejgallego pushed a commit that referenced this issue Feb 9, 2024
Fixes #9818

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: d2fe6b57-4512-4907-8577-43fdabcdcf8b -->
ejgallego pushed a commit that referenced this issue Feb 9, 2024
Fixes #9818

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: d2fe6b57-4512-4907-8577-43fdabcdcf8b -->
@rgrinberg rgrinberg added this to the 3.14.0 milestone Feb 10, 2024
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this issue Mar 26, 2024
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this issue Apr 3, 2024
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment