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

[coqdep] Don't "canonize" files. #16116

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ejgallego
Copy link
Member

"Canonization" is a fragile (and expensive) process, instead, we
require that files passed to coqdep match the layout given in the -R
/ -Q options.

@ejgallego ejgallego requested a review from a team May 31, 2022 18:25
@ejgallego ejgallego requested a review from a team as a code owner May 31, 2022 18:25
@ejgallego ejgallego added kind: cleanup Code removal, deprecation, refactorings, etc. needs: changelog entry This should be documented in doc/changelog. part: coqdep The coqdep binary for resolving dependencies amongst coq .v files. labels May 31, 2022
@Alizter Alizter added this to In progress in coqdep via automation May 31, 2022
@Alizter Alizter self-assigned this May 31, 2022
@Alizter Alizter added this to the 8.17+rc1 milestone May 31, 2022
@ejgallego
Copy link
Member Author

For reference, the canonization tries to solve a hard to solve problem: make can't understand directory traversal properly, so for make foo/.. and . are different directories. The right way to solve this is for coqdep to do nothing with the paths, and just require users to input to coq_makefile consistent paths.

Note that make does recognize ./foo and foo as identical, and a few other ones.

See mit-plv/coqutil#67 for more info.

@gares
Copy link
Member

gares commented Jun 24, 2022

foo/.. and . are not necessarily the same path if foo is a symlink. you have to look beyondthe syntax to know if it is the case.

@ejgallego
Copy link
Member Author

Yes, indeed that's not the best example, but this is why canonization is a complex process. (And that's a big problem for submakefiles, dune has to deal with a lot of stuff so composition properly works due to adjusting paths)

It is better just to have the invariant that coq_makefile and coqdep don't do that, and just have to share the same path prefix, so we don't even try to understand if foo/.. and . are the same dir or not (without canonization, they are just not)

@github-actions github-actions bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Jun 28, 2022
@ejgallego ejgallego added this to Ready to address in Dune Jul 7, 2022
@Alizter
Copy link
Contributor

Alizter commented Jul 9, 2022

Can you rebase and fix the CI?

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jul 28, 2022

The "needs: rebase" label was set more than 30 days ago. If the PR is not rebased in 30 days, it will be automatically closed.

@coqbot-app coqbot-app bot added the stale This PR will be closed unless it is rebased. label Jul 28, 2022
"Canonization" is a fragile (and expensive) process, instead, we
require that files passed to coqdep match the layout given in the `-R`
/ `-Q` options.
@coqbot-app coqbot-app bot removed needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. stale This PR will be closed unless it is rebased. labels Jul 28, 2022
@ejgallego ejgallego removed this from the 8.17+rc1 milestone Nov 17, 2022
@Alizter Alizter removed their assignment Dec 11, 2022
@SkySkimmer
Copy link
Contributor

@coqbot run full ci

@SkySkimmer SkySkimmer requested a review from a team April 17, 2024 13:46
@ejgallego
Copy link
Member Author

@SkySkimmer last time this PR was run indeed some users did rely on passing incorrect paths to Coq makefile but coqdep fixing them.

Maybe we should go ahead and do some overlays, I dunno.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: cleanup Code removal, deprecation, refactorings, etc. needs: changelog entry This should be documented in doc/changelog. part: coqdep The coqdep binary for resolving dependencies amongst coq .v files.
Projects
Dune
  
Ready to address
coqdep
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants