Skip to content

Allow dune subst to work inside a subfolder of a Git repository#11760

Merged
rgrinberg merged 4 commits intoocaml:mainfrom
Richard-Degenne:fix-11045-git-subfolders
May 7, 2025
Merged

Allow dune subst to work inside a subfolder of a Git repository#11760
rgrinberg merged 4 commits intoocaml:mainfrom
Richard-Degenne:fix-11045-git-subfolders

Conversation

@Richard-Degenne
Copy link
Copy Markdown
Contributor

@Richard-Degenne Richard-Degenne commented May 5, 2025

I gave my best shot at fixing #11045, but tests fail with a runtime error that I have a hard time understanding. If anyone could give me some pointers or complete the fix, that would be awesome.

Internal error, please report upstream including the contents of _build/log.
Description:
  ("Fdecl.get: not set", {})
Raised at Stdune__Code_error.raise in file
  "otherlibs/stdune/src/code_error.ml", line 10, characters 30-62
Called from Dune_rules__Only_packages.Clflags.t in file
  "src/dune_rules/only_packages.ml" (inlined), line 25, characters 13-24
Called from Dune_rules__Only_packages.filter_packages_in_project in file
  "src/dune_rules/only_packages.ml", line 68, characters 8-20
Called from Fiber__Core.O.(>>|).(fun) in file "vendor/fiber/src/core.ml",
  line 253, characters 36-41
Called from Fiber__Scheduler.exec in file "vendor/fiber/src/scheduler.ml",
  line 76, characters 8-11
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/src/exn.ml", line 38, characters 27-56
Called from Fiber__Scheduler.exec in file "vendor/fiber/src/scheduler.ml",
  line 76, characters 8-11
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/src/exn.ml", line 38, characters 27-56
Called from Fiber__Scheduler.exec in file "vendor/fiber/src/scheduler.ml",
  line 76, characters 8-11
-> required by ("find-dir-raw", In_source_tree ".")

@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented May 5, 2025 •
edited
Loading

I believe the issue is coming from (ln 309 in subst.ml):

  (* We have to do this because scanning the source tree evaluates [-p].
    That's because [-p] is needed to interpret packages in dune projects
    correctly. It should not be necessary, so we should probably make the
    package loading lazier. *)
  Dune_rules.Only_packages.Clflags.set No_restriction;

It looks to me like that this doesn't get set if there is a vcs present. We probably want that to be set in both cases by moving this before the match if we want Source_tree to work correctly. Not sure if this will fix all the issues however.

Edit: Yes, that appears to have worked. I would move it (together with the comment) to the term function where we set other stuff.

@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented May 5, 2025 •
edited
Loading

Another comment, it would be great if you could include a commit before with a reproduction cram test. That we can be sure this commit will fix the issue and have a nice commit history.

You want to add the test in test/blackbox-tests/test-cases/subst/.

Let me know if you need any help.

@Richard-Degenne
Copy link
Copy Markdown
Contributor Author

Richard-Degenne commented May 5, 2025

Awesome, thank you so much for the quick reply. I'll see what I can do for the test.

@Richard-Degenne
Copy link
Copy Markdown
Contributor Author

Richard-Degenne commented May 5, 2025

This is my first time using cram, but the test seems to be working: it fails without the fix, but passes afterwards.

Let me know if there is anything I should have done differently.

Side question: is there a way to run a single test file? I've been running make test every time, it's kind of sluggish 😅

@Richard-Degenne Richard-Degenne marked this pull request as ready for review May 5, 2025 19:42
@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented May 5, 2025

You can run a single cram test by building the alias given by its name. Simply do ./dune.exe build @git-subfolder to build the test. Another tip is to use watch mode -w and --auto-promote which will automatically promote the current output of the test so that it succeeds. When you update the test, dune will make sure the expected output is updated.

Good first try, just a couple of comments:

  • Usually we want to test to always succeed, even if it outputs an error in the test itself.
  • We try to keep the reproduction case small so it is clearer which parts are causing issues. dune init does a bit more than is necessary so I've gone ahead and stripped out the important parts.

You should update the first commit to have this cram test:

Running `dune subst` in a subfolder of a Git repository should work.
Regression test for https://github.com/ocaml/dune/issues/11045

  $ git init --quiet

  $ mkdir subfolder
  $ cat > subfolder/dune-project <<EOF
  > (lang dune 3.19)
  > (name subfolder)
  > EOF

  $ cat > subfolder/subfolder.opam

`dune subst` requires at least one commit in the repository.

  $ git add .
  $ git commit --quiet --message "Initial commit"
  $ cd subfolder
  $ dune subst 2>&1 | grep "Internal error: Uncaught exception"
  | Internal error: Uncaught exception.

In the second commit, you will of course have the updated output.

@Richard-Degenne
Copy link
Copy Markdown
Contributor Author

Richard-Degenne commented May 7, 2025

@rgrinberg: Thank you for completing the fix! Is there anything else that needs to be done before the PR can be considered for merging?

Richard-Degenne and others added 4 commits May 7, 2025 20:59
@Richard-Degenne @rgrinberg
test: Added regression test for #11045
07fb215 
Signed-off-by: Richard Degenne <richard.degenne+github@gmail.com>
@rgrinberg
fix: error messages when unable to substitute version
222c306 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@Richard-Degenne @rgrinberg
fix: $ dune subst in sub-directories of a repo
56a6a79 
Fixes #11045

Signed-off-by: Richard Degenne <richard.degenne+github@gmail.com>
@rgrinberg
fix: correctly list vcs files in subdirectory
16f442b 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg merged commit d556757 into ocaml:main May 7, 2025
10 of 13 checks passed
@Richard-Degenne Richard-Degenne deleted the fix-11045-git-subfolders branch May 9, 2025 15:37
@maiste maiste mentioned this pull request May 20, 2025
Closed
maiste added a commit to maiste/opam-repository that referenced this pull request May 20, 2025
@maiste
[new release] dune (15 packages) (3.19.0~alpha0)
84ab298 
CHANGES:

### Fixed

- Fixed a bug that was causing cram tests attached to multiple aliases to be run multiple
  times. (ocaml/dune#11547, @Alizter)

- Fix: pass pkg-config (extra) args in all pkgconfig invocations. A missing --personality
  flag would result in pkgconf not finding libraries in some contexts. (ocaml/dune#11619, @MisterDA)

- Fix: Evaluate `enabled_if` when computing the stubs for stanzas such as
  `foreign_library` (ocaml/dune#11707, @Alizter, @rgrinberg)

- Fix $ dune describe pp for libraries in the presence of `(include_subdirs
  unqualified)` (ocaml/dune#11729, fixes ocaml/dune#10999, @rgrinberg)

- Fix `$ dune subst` in sub directories of a git repository (ocaml/dune#11760, fixes
  ocaml/dune#11045, @Richard-Degenne)

- Fix a crash involving `Path.drop_prefix` when using Melange on Windows
  (ocaml/dune#11767, @nojb)

### Added

- Added detection and warning for common typos in package dependency
  constraints (ocaml/dune#11600, fixes ocaml/dune#11575, @kemsguy7)

- Added `(extra_objects)` field to `(foreign_library)` stanza with `(:include)` support.
  (ocaml/dune#11683, @Alizter)

### Changed

- Allow build RPC messages to be handled by dune's RPC server in eager watch
  mode (ocaml/dune#11622, @gridbugs)

- Allow concurrent build with RPC server (ocaml/dune#11712, @gridbugs)
maiste added a commit to maiste/opam-repository that referenced this pull request May 21, 2025
@maiste
[new release] dune (15 packages) (3.19.0)
f85c940 
CHANGES:

### Fixed

- Fixed a bug that was causing cram tests attached to multiple aliases to be run multiple
  times. (ocaml/dune#11547, @Alizter)

- Fix: pass pkg-config (extra) args in all pkgconfig invocations. A missing --personality
  flag would result in pkgconf not finding libraries in some contexts. (ocaml/dune#11619, @MisterDA)

- Fix: Evaluate `enabled_if` when computing the stubs for stanzas such as
  `foreign_library` (ocaml/dune#11707, @Alizter, @rgrinberg)

- Fix $ dune describe pp for libraries in the presence of `(include_subdirs
  unqualified)` (ocaml/dune#11729, fixes ocaml/dune#10999, @rgrinberg)

- Fix `$ dune subst` in sub directories of a git repository (ocaml/dune#11760, fixes
  ocaml/dune#11045, @Richard-Degenne)

- Fix a crash involving `Path.drop_prefix` when using Melange on Windows
  (ocaml/dune#11767, @nojb)

### Added

- Added detection and warning for common typos in package dependency
  constraints (ocaml/dune#11600, fixes ocaml/dune#11575, @kemsguy7)

- Added `(extra_objects)` field to `(foreign_library)` stanza with `(:include)` support.
  (ocaml/dune#11683, @Alizter)

### Changed

- Allow build RPC messages to be handled by dune's RPC server in eager watch
  mode (ocaml/dune#11622, @gridbugs)

- Allow concurrent build with RPC server (ocaml/dune#11712, @gridbugs)
@maiste maiste mentioned this pull request May 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Richard-Degenne @Alizter @rgrinberg