Add case sensitivity-check to computeCommonSourceDirectory#5590
Conversation
…ified and resource based paths are found
There was a problem hiding this comment.
use getCanonicalFileName instead
There was a problem hiding this comment.
Rolled back using getCanonicalFileName - the change in output this caused was a nightmare for our projects tests. On first attempt, the canonical path becomes absolute in many cases - this was a big change which would pretty much require reworking the test harness' path handling, so then I tried patching the function to make the result relative if need be - this was still a change in output, because the resulting relative paths could be different than the ones found by looking for common path components (usually they'd end up with more common components). This caused absolute paths to be generated by other algorithms elsewhere which then got emitted in tests and, generally, it was a mess.
We should really swap to using fully qualified Path types for everything and convert to root-relative paths for test output (in the harness).
There was a problem hiding this comment.
And now I'm using getCanonicalFileName again. 👍
Worked around the casing handling in the harness/emitter by using getCanonicalFileName on the path fragments when comparing fragments rather than on the whole initial path. This is similar to what we do within a few of our utility functions, so I suppose this use of getCanonicalFileName is to be expected, though I didn't expect it to be handled at first.
There was a problem hiding this comment.
I'd extract this to a function named something like prefixRootDir(name, root) or maybe prefixRootDirIfNeeded.
|
Once this is merged our remaining RWC baseline failures seem to be a removal of a handful of errors regarding duplicate implementations (not sure of the cause) and some sourcemap changes (Likely from #5643). |
There was a problem hiding this comment.
Scratch that, no - we're saying > 1 because we want to exclude paths which start with /, which isRootedDiskPath would include.
There was a problem hiding this comment.
Since if the root is a filesystem rooted at /, we can just use / as the common dir.
There was a problem hiding this comment.
but apparently we cannot do this since commonSourceDirectory is "". Just trying to understand in what cases using isRootedDiskPath will yield incorrect results.
There was a problem hiding this comment.
Correct - in effect, we want to allow "" as a valid common source directory provided either all paths are relative or all paths are rooted to a unix-style drive (with a root of /), otherwise "" is the return value when there is no available common path and we should error in its presence.
|
👍 |
Add case sensitivity-check to computeCommonSourceDirectory
Also only error on failure when outDir is specified and resource based paths are found.
Fixes #5581.