Skip to content

Conversation

@weswigham
Copy link
Member

Also only error on failure when outDir is specified and resource based paths are found.

Fixes #5581.

Copy link
Contributor

Choose a reason for hiding this comment

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

use getCanonicalFileName instead

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I'd extract this to a function named something like prefixRootDir(name, root) or maybe prefixRootDirIfNeeded.

@weswigham
Copy link
Member Author

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

isRootedDiskPath ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Aye, that works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Scratch that, no - we're saying > 1 because we want to exclude paths which start with /, which isRootedDiskPath would include.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since if the root is a filesystem rooted at /, we can just use / as the common dir.

Copy link
Contributor

Choose a reason for hiding this comment

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

but apparently we cannot do this since commonSourceDirectory is "". Just trying to understand in what cases using isRootedDiskPath will yield incorrect results.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

discussed offline

@vladima
Copy link
Contributor

vladima commented Nov 18, 2015

👍

weswigham added a commit that referenced this pull request Nov 18, 2015
Add case sensitivity-check to computeCommonSourceDirectory
@weswigham weswigham merged commit 1f61ecf into microsoft:master Nov 18, 2015
@weswigham weswigham deleted the compute-common-source-dir branch August 17, 2017 23:04
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants