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.

Sign up for GitHub

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

Jump to bottom

[regression] promotion of directory targets has trouble with directories starting in @ #10609

Open
ejgallego opened this issue Jun 4, 2024 · 10 comments · May be fixed by #10931
Open

[regression] promotion of directory targets has trouble with directories starting in @ #10609

ejgallego opened this issue Jun 4, 2024 · 10 comments · May be fixed by #10931

Comments

@ejgallego
Copy link
Collaborator

Dear dune devs,

updating to Dune 3.15, I've found the following problem with this simple rule (that used to work):

(rule
 (targets
   (dir node_modules))
 (deps package.json)
 (mode promote)
 (action
  (bash "npm i")))

it seems in 3.15.3, dune chokes with:

Error: Cannot promote files to "node_modules/@babel/parser".
Reason: opendir(node_modules/@babel/parser): No such file or directory
-> required by _build/default/node_modules

File is correctly present in _build dir.

Any package.json file that depends on a package starting with @ seems to do the trick, for example:

{
  "name": "test",
  "version": "0.17.0",
  "type": "module",
  "description": "NPM test",
  "main": "index.js",
  "dependencies": {
    "@octokit/core": "^4.2.1"
  }
}

Bisect points to b1c339b , I've verified this to be the culprit cc @rleshchinskiy
@emillon
Copy link
Collaborator

emillon commented Jun 5, 2024

it's interesting because the bug does not trigger with just "mkdir node_modules/@x/y; touch node_module/@x/y/z.txt"

@ejgallego
Copy link
Collaborator Author

Indeed, I wonder if my npm is doing some hardlinking on its own? Just in case npm --version = 9.6.7

@ejgallego
Copy link
Collaborator Author

Nothing weird on that side, stat reports entries in path are regular dirs and files.

@emillon
Copy link
Collaborator

emillon commented Jun 5, 2024

Yes, I managed to repro with npm too.

stat reports entries in path are regular dirs and files.

To clarify, hardlinks are regular files; did you check the link count too?

@ejgallego
Copy link
Collaborator Author

They have link count larger than 1, I see:

find ~ -samefile _build/default/node_modules/@octokit/auth-token/LICENSE 
/home/egallego/.cache/dune/db/files/v4/dd/dd65cb7fe001574466341c3a0618de18
/home/egallego/tmp/dune-bugs/dune-promote/_build/default/node_modules/@octokit/core/LICENSE
/home/egallego/tmp/dune-bugs/dune-promote/_build/default/node_modules/@octokit/auth-token/LICENSE
/home/egallego/tmp/dune-bugs/dune-promote/_build/default/node_modules/@octokit/request-error/LICENSE

@emillon
Copy link
Collaborator

emillon commented Jun 6, 2024

FTR, #9873 points to the same commit, but it's a bit different, it's when a symlink points to a dir.

@ejgallego
Copy link
Collaborator Author

Indeed, symlinks were an obvious suspect, but I can't see any involved here (so far)

@emillon
Copy link
Collaborator

emillon commented Jun 7, 2024

one thing I haven't checked is if the issue happens when no directory starts with @. maybe that part is just a red herring?

@ejgallego
Copy link
Collaborator Author

I have checked, if the @ symbol is not in the path, promotion seems to work fine. So it seems related.

@ejgallego
Copy link
Collaborator Author

ejgallego commented Sep 17, 2024 •
edited
Loading

I've tracked this to the part of b1c339b that avoids adding "empty" directories:

dune/src/dune_targets/dune_targets.ml

Lines 246 to 248 in 4f86203

if not (Filename.Map.is_empty filenames)
then Path.Local.Map.add_exn dirs local filenames
else dirs

I am trying to understand what is wrong here.

ejgallego added a commit to ejgallego/dune that referenced this issue Sep 17, 2024
@ejgallego
[fix] [directory targets] Fix promotion of directory targets in some …
7888885 
…cases

When the promoted directory has sub-directories with only other
sub-directories inside, the map will miss that directory and it will
not be properly re-created, failing at children creation time.

I am not sure I fully follow the logic here (cc: @rleshchinskiy), for
example, I did have trouble making a testcase fail without using `npm`
to generate a large directory.

Regression in b1c339b / ocaml#9407

Closes ocaml#10609 .

Signed-off-by: Emilio Jesus Gallego Arias <[email protected]>
@ejgallego ejgallego linked a pull request Sep 17, 2024 that will close this issue
Open
ejgallego added a commit to ejgallego/dune that referenced this issue Sep 17, 2024
@ejgallego
[fix] [directory targets] Fix promotion of directory targets in some …
5019bce 
…cases

When the promoted directory has sub-directories with only other
sub-directories inside, the map will miss that directory and it will
not be properly re-created, failing at children creation time.

I am not sure I fully follow the logic here (cc: @rleshchinskiy).

The current test case works for me (TM), if we revert the fix we get:

```
Error: Cannot promote files to "node_modules/node-cmake/node_modules/ansi-regex".
Reason: opendir(node_modules/node-cmake/node_modules/ansi-regex): No such file or directory
```

Regression in b1c339b / ocaml#9407

Closes ocaml#10609 .

Signed-off-by: Emilio Jesus Gallego Arias <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[fix] [directory targets] Fix promotion of directory targets in some cases ejgallego/dune
2 participants
@emillon @ejgallego