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

Fix ocamldep-postproc in case-insensitive, case-preserving filesystems. #150

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

Conversation

mfp
Copy link

@mfp mfp commented Jul 1, 2022

On case-insensitive, case-preserving filesystems, omake finds invalid dependencies corresponding to paths that do not exist on disk since they have the wrong capitalization.

In particular, given module A that depends on B, and the corresponding source files a.ml and b.ml, if you have a target like OCamlPackage(runtime, a b), omake will invoke ocamlc as in ocamlc -pack -o runtime.cmo B.cmo a.cmo. Now, on a case-insensitive, case-preserving filesystem, b.cmo can be open(2)ed under both b.cmo and B.cmo paths, but ocamlc fails -- it must be doing readdir and sees B.cmo is missing.

This is caused by a bug in the ocamldep-postproc builtin. I verified this by overriding its definition in an affected codebase (extprot) under macOS to use OCamlScannerPostproc, which did work.

This PR fixes the builtin itself.

Credits

The work on this PR was performed on behalf of Ahrefs open-source work day:
https://twitter.com/javierwchavarri/status/1466774912148910088

[name1; name2]
else
[name2; name1] in
let names = (* check name with uncapitalized case first *) [name1; name2] in
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was required for omake's bootstrapping to work.

let unix_filename_exists path =
let basedir = Filename.dirname path in
let basename = Filename.basename path in
basename = "." || basename = ".." || Array.exists (String.equal basename) (Sys.readdir basedir)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readdir inside stat (wrapper) sounds expensive

Copy link
Author

@mfp mfp Jul 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is :-(, but I'm not aware of any BSD/POSIX syscall/API to obtain the original capitalization -- I read even realpath(3) lies. And I don't know how to determine whether the filesystem is case-insensitive to restrict the above check to that case (without write ops that is). At least it's performed only when there's a successful (l)stat.

The only alternative I can think of is finding the mount point, probing the FS and caching this info, but it still feels a bit dirty.

@gerdstolpmann
Copy link
Collaborator

I'd rather say that this is a bug/problem in ocamlc. We really cannot afford to do a readdir on each stat.

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 this pull request may close these issues.

3 participants