Calculate correct relative paths from fsatrace output. #656
Conversation
| @@ -145,7 +145,8 @@ commandExplicit funcName oopts results exe args = do | |||
| shelled = runShell (unwords $ exe : args) | |||
|
|
|||
| ignore = map (?==) shakeLintIgnore | |||
| ham cwd xs = [makeRelative cwd x | x <- map toStandard xs | |||
| ham cwd xs = liftIO $ sequence [fromMaybe x <$> makeRelativeEx cwd x | |||
There was a problem hiding this comment.
I'm concerned with performance here. Thoughts?
There was a problem hiding this comment.
We only call this after spawning a process. Hard to do much that has a performance impact relative to that.
| @@ -145,7 +145,8 @@ commandExplicit funcName oopts results exe args = do | |||
| shelled = runShell (unwords $ exe : args) | |||
|
|
|||
| ignore = map (?==) shakeLintIgnore | |||
| ham cwd xs = [makeRelative cwd x | x <- map toStandard xs | |||
| ham cwd xs = liftIO $ sequence [fromMaybe x <$> makeRelativeEx cwd x | |||
There was a problem hiding this comment.
What does ham actually mean?
There is an underlying assumption here that track uses relative paths as keys. Is that correct? It's troubling to me that there may be many ways to refer to the same file (relative, absolute, via different symlinks or indirections), yet there doesn't seem to be any mechanism to enforce a canonical usage.
|
Thanks for the patch. Can you explain the output you get before this patch? e.g. when running you see XXX, and we're relative to YYY, and you hoped to make it ZZZ but it failed? We should generally in Shake be referring to things by relative paths. If you have symlinks, you are going to have problems with build systems. If you use absolute paths its much harder to be portable for features like cloud shake. Relative paths everywhere solves these problems nicely. |
DavidEichmann
force-pushed
the
fsatrace-lint
branch
from
c43f196
to
22ab6d8
Compare
|
I've added a bit of documentation to the commit message and the |
DavidEichmann
force-pushed
the
fsatrace-lint
branch
3 times, most recently
from
32b28fa
to
708bfd6
Compare
|
Neil, I wanted to add some tests for the new |
|
Hopefully https://github.com/ndmitchell/shake/blob/master/docs/Developing.md answers your questions. Simple answer is add the word |
|
Sorry, I should have clarified what I was looking for. It's clear what |
BEFORE this MR(*) I get the following when building my local GHC branch (includes changes to enable fsatrace linting over the build directory) : In particular, note that I am using a build directory outside of the source tree: (**) Now if I correctly add a need statement in the corresponding rule (using a relative path as is the convention): The BUG surfaces: this does not fix the linting error. Needing the absolute path does fix the linting error, but is not a satisfactory solution. AFTER this MRWith Hadrian as in (*) I do: The number of errors decreased from 36 to 31. This is because Hadrian already correctly needs some of the files (using relative paths). Note also that the linting error gives relative paths thanks to To Summarizefsatrace linting fails when the accessed files are not under the current working directory even if the dependency is coded correctly in the build system (using a relative path). Does this answer your question? |
With fsatrace linting, a command that writes a file, unknown to
the build system, should then cause a linting error if that file
is later `need`ed. Previously, shake would fail to give this
expected linting error when the file in question if not under
the current working directory.
The reason for this is fsatrace gives absolute file paths and
`makeRelative` will often fail to convert to a relative path e.g:
> makeRelative "/root/a/" "/root/b/file.out"
"/root/b/file.out"
This is because `makeRelative` is pure and cannot guarantee a
correct relative path incase "a" is a symlink (i.e.
"/root/a/../b/file.out" is not necessarily equal to
"/root/b/file.out"). This commit fixes the bug by introducing the
function `makeRelativeEx` that guarantees a relative path is found
when one exists (at the cost of being impure).
> -- Given that "/root/a/" is not a symlink
> makeRelativeEx "/root/a/" "/root/b/file.out"
Just "../b/file.out"
> -- Given that "/root/c/" is a symlink to "/root/e/f/g/"
> makeRelativeEx "/root/c/" "/root/b/file.out"
Just "../../../b/file.out"
DavidEichmann
force-pushed
the
fsatrace-lint
branch
from
708bfd6
to
2286d2a
Compare
|
I've added some test cases now. Ready for review. |
|
OK, so the magic ingredient is that you are using a relative path outside the current directory for the build. That's not something I ever really consider - but no reason it shouldn't work. Thanks for the patch! |
|
I followed up with a pile of tests and fixes that means it should work robustly now. I'll also write proper docs for this lint mode! |
I've adding this PR in relation to GHC trac issue 16400.