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

Strange error with directory targets #6168

Closed
ejgallego opened this issue Sep 28, 2022 · 13 comments · Fixed by #6219
Closed

Strange error with directory targets #6168

ejgallego opened this issue Sep 28, 2022 · 13 comments · Fixed by #6219
Milestone

Comments

@ejgallego
Copy link
Collaborator

Hi folks, using this simple dune file:

(rule
 (targets (dir node_modules))
 (deps package.json)
 (action (run npm install --no-save)))

(alias
 (name node)
 (deps node_modules))

dune build @node fails with

File "dune", line 1, characters 0-95:
1 | (rule
2 |  (targets (dir node_modules))
3 |  (deps package.json)
4 |  (action (run npm install --no-save)))
Error: This rule defines a directory target "node_modules" that matches the
requested path "node_modules" but the rule's action didn't produce it
-> required by alias node in dune:6

however npm install generates the directory just fine; this is in today's main. Adding sandboxing doesn't alter the result.

@Alizter
Copy link
Collaborator

Alizter commented Sep 28, 2022

Do you have directory targets enabled?

@ejgallego
Copy link
Collaborator Author

Yup dune-project is:

(lang dune 3.4)
(using directory-targets 0.1)

@emillon
Copy link
Collaborator

emillon commented Sep 29, 2022

to confirm, the action ran in _build/default and created _build/default/node_modules/?

@ejgallego
Copy link
Collaborator Author

Yes @emillon (it is very easy to reproduce, so if someone can confirm it'd be great)

@ejgallego ejgallego added this to the 3.5.0 milestone Sep 29, 2022
@ejgallego
Copy link
Collaborator Author

I'm adding this to the milestone as it could be understood as a regression.

@Alizter
Copy link
Collaborator

Alizter commented Sep 29, 2022

@ejgallego Does it work on a previous version of Dune?

@ejgallego
Copy link
Collaborator Author

Yes (tho 3.3 didn't like the symlinks inside node, but we just clean them up and that works)

@ejgallego
Copy link
Collaborator Author

@rgrinberg if that serves as a clue, it seems still related to links somehow, the following action still works fine on main:

 (action
  (progn
   (run npm install --no-save)
   (run find node_modules -type l -exec rm {} ";"))))

Alizter added a commit to Alizter/dune that referenced this issue Oct 5, 2022
Signed-off-by: Ali Caglayan <alizter@gmail.com>

ps-id: 81001677-82b6-4bb8-bf41-64dd64613eae
Alizter added a commit to Alizter/dune that referenced this issue Oct 6, 2022
Signed-off-by: Ali Caglayan <alizter@gmail.com>

ps-id: 81001677-82b6-4bb8-bf41-64dd64613eae
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
Copy link
Collaborator

Alizter commented Oct 6, 2022

Repro case here #6189

@ejgallego
Copy link
Collaborator Author

Thanks a lot @Alizter ! This was a tricky one actually

@Alizter
Copy link
Collaborator

Alizter commented Oct 6, 2022

The digest computation for directory targets is broken because when it recuses on the subdirectory it forgets that symlinks are allowed inside and fails instead.

Alizter added a commit to Alizter/dune that referenced this issue Oct 6, 2022
Signed-off-by: Ali Caglayan <alizter@gmail.com>

ps-id: 81001677-82b6-4bb8-bf41-64dd64613eae
Signed-off-by: Ali Caglayan <alizter@gmail.com>
Alizter added a commit to Alizter/dune that referenced this issue Oct 6, 2022
Signed-off-by: Ali Caglayan <alizter@gmail.com>

ps-id: 81001677-82b6-4bb8-bf41-64dd64613eae
Signed-off-by: Ali Caglayan <alizter@gmail.com>
Alizter added a commit to Alizter/dune that referenced this issue Oct 6, 2022
Signed-off-by: Ali Caglayan <alizter@gmail.com>

ps-id: 81001677-82b6-4bb8-bf41-64dd64613eae
Signed-off-by: Ali Caglayan <alizter@gmail.com>
Alizter added a commit to Alizter/dune that referenced this issue Oct 6, 2022
Signed-off-by: Ali Caglayan <alizter@gmail.com>

ps-id: 81001677-82b6-4bb8-bf41-64dd64613eae
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@emillon
Copy link
Collaborator

emillon commented Oct 11, 2022

As this is related to directory targets which are experimental, we said that it would be fine to release 3.5 without a fix to this.

@emillon emillon modified the milestones: 3.5.0, 3.6.0 Oct 11, 2022
rgrinberg pushed a commit to Alizter/dune that referenced this issue Oct 12, 2022
Signed-off-by: Ali Caglayan <alizter@gmail.com>

ps-id: 81001677-82b6-4bb8-bf41-64dd64613eae
Signed-off-by: Ali Caglayan <alizter@gmail.com>
rgrinberg pushed a commit that referenced this issue Oct 12, 2022
Signed-off-by: Ali Caglayan <alizter@gmail.com>

ps-id: 81001677-82b6-4bb8-bf41-64dd64613eae
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@rgrinberg rgrinberg linked a pull request Oct 12, 2022 that will close this issue
@rgrinberg rgrinberg modified the milestones: 3.6.0, 3.5.0 Oct 12, 2022
@rgrinberg
Copy link
Member

Bumping this back to 3.5.0 as I have a fix ready.

@emillon emillon modified the milestones: 3.5.0, 3.6.0 Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants