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

[local-cli] Support symlinks-to-symlinks #9792

Closed
wants to merge 1 commit into from

Conversation

@aleclarson
Copy link
Contributor

commented Sep 7, 2016

In response to this comment.

I could be wrong here, but I think Watchman can't handle symlinks, so we need to make sure symlinks-to-symlinks are handled up front.

edit: Here's a summary of the changes made by this commit:

  • Squash symlink resolution into a single forEach
  • Resolve symlinks recursively
  • Throw an error for circular symlinks
  • Use lookupFolder to resolve folder (instead of process.cwd())
  • Resolve relative symlinks correctly
  • Simplify isSubPathOfPath function (and rename to isDescendant)
  • Rename isSubPathOfPaths function to rootExists
  • Rename existingSearchPaths argument to ignoredRoots
  • Pass both args.projectRoots and args.root to findSymlinksPaths
@ghost

This comment has been minimized.

Copy link

commented Sep 7, 2016

By analyzing the blame information on this pull request, we identified @Kureev to be a potential reviewer.

@mkonicek

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2016

Thanks for the PR. I'll let @Kureev take a look as he's added the symlink support (thank you for doing that!).

@ghost ghost added the CLA Signed label Sep 8, 2016

@Kureev

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2016

So, recursive lookup for symlinks? Nice idea!

offtopic: haven't seen do...while for a errrm while already
@Kureev

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2016

IMHO,

do {
  symlink = path.resolve(process.cwd(), fs.readlinkSync(symlink));
} while (fs.lstatSync(symlink).isSymbolicLink());

is a bit harder to grasp than

var recursiveSymlink;
while (fs.lstatSync(symlink).isSymbolicLink()) {
  recursiveSymlink = path.resolve(process.cwd(), fs.readlinkSync(symlink))
};

this version has two improvements from my perspective:

  • we don't override iterator argument
  • while {} seems to be more popular construction then do {} while

Also, if you want to solve recursive symlink problem, you probably have to think about loops:
A -> B -> A. I propose to store a linked list of "visits" and check if current symlink path is not in it, otherwise throw.

What do you think, guys? Does it make any sense to you?

@ghost

This comment has been minimized.

Copy link

commented Sep 9, 2016

@aleclarson updated the pull request - view changes


prevSymlink = nextSymlink;
nextSymlink = path.resolve(
path.dirname(prevSymlink),

This comment has been minimized.

Copy link
@aleclarson

aleclarson Sep 9, 2016

Author Contributor

Note: Symlinks are now resolved relative to their parent directory. This is the correct behavior, because symlinks aren't aware of process.cwd() and thus could be resolved into the wrong path.

@ghost ghost added the CLA Signed label Sep 9, 2016

const symlinkCache = new Set();
let prevSymlink;
let nextSymlink = symlink;
while (true) {

This comment has been minimized.

Copy link
@aleclarson

aleclarson Sep 9, 2016

Author Contributor

while (true) is used here because we know for a fact that symlink is a symlink.
At the end of each loop, !fs.lstatSync(nextSymlink).isSymbolicLink() determines when to stop.

This comment has been minimized.

Copy link
@Kureev

Kureev Sep 9, 2016

Collaborator

Why not

while (fs.lstatSync(nextSymlink).isSymbolicLink()) {...}

?

This comment has been minimized.

Copy link
@aleclarson

aleclarson Sep 9, 2016

Author Contributor

Seems redundant, no? We already do the filter call to make sure only symlinks are present.

@@ -6,7 +6,28 @@ module.exports = function findSymlinksPaths(lookupFolder) {
const folders = fs.readdirSync(lookupFolder);
const resolvedSymlinks = folders.map(folder => path.resolve(lookupFolder, folder))
.filter(folderPath => fs.lstatSync(folderPath).isSymbolicLink())
.map(symlink => path.resolve(process.cwd(), fs.readlinkSync(symlink)));
.map(symlink => {
const symlinkCache = new Set();

This comment has been minimized.

Copy link
@aleclarson

aleclarson Sep 9, 2016

Author Contributor

We can easily detect symlink recursion just by caching each resolved symlink.
Though we could improve the error message by (1) keeping track of the resolution order and (2) printing the symlinks that were resolved in between the first & second appearances of the faulty symlink. This would help the end user fix the recursion.

@aleclarson

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2016

@Kureev Let me know what you think of the new changes. 😄

I'll squash the commits once everything is okay'd.

@ghost ghost added the CLA Signed label Sep 9, 2016

let nextSymlink = symlink;
while (true) {
if (symlinkCache.has(nextSymlink)) {
throw Error(`Symlink recursion detected:\n ${nextSymlink}`);

This comment has been minimized.

Copy link
@Kureev

Kureev Sep 9, 2016

Collaborator

Will be really great if we can throw an error with the whole sequence that leads to the recursion. Like "Symlink recursion detected: A -> B -> C -> B"

This comment has been minimized.

Copy link
@aleclarson

aleclarson Sep 9, 2016

Author Contributor

Truth. I'll just switch to using an Array and then use slice to get the relevant sequence.

Though it could get a little messy if the sequence is long.

}
symlinkCache.add(prevSymlink);

prevSymlink = nextSymlink;

This comment has been minimized.

Copy link
@Kureev

Kureev Sep 9, 2016

Collaborator

Can you explain this assignment to me?

This comment has been minimized.

Copy link
@aleclarson

aleclarson Sep 9, 2016

Author Contributor

Just realized prevSymlink isnt even necessary. 😛

@Kureev

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2016

In general, looks really good. I think we can merge it in once we agree on some minor details

@facebook-github-bot

This comment has been minimized.

Copy link

commented Sep 9, 2016

@aleclarson updated the pull request - view changes

);
}

if (visited.length) {

This comment has been minimized.

Copy link
@aleclarson

aleclarson Sep 9, 2016

Author Contributor

Since we no longer call folders.filter to guarantee each item is a symlink, we need to protect against while (fs.lstatSync(symlink).isSymbolicLink()) doing zero loops.

@ghost ghost added the CLA Signed label Sep 9, 2016

if (index !== -1) {
throw Error(
`Infinite symlink recursion detected:\n ` +
visited.slice(index).join(`\n `)

This comment has been minimized.

Copy link
@aleclarson

aleclarson Sep 9, 2016

Author Contributor

Note: This prints the symlink sequence in order, as opposed to being reversed.
Not sure which way is optimal, though.

@Kureev

This comment has been minimized.

Copy link
Collaborator

commented Sep 12, 2016

@Kureev

This comment has been minimized.

Copy link
Collaborator

commented Sep 12, 2016

Thank you for your contribution, @aleclarson! I think it's good to go now 👍

@aleclarson aleclarson force-pushed the aleclarson:symlinks-to-symlinks branch from e27139a to 165f1ba Sep 12, 2016

@ghost ghost added the CLA Signed label Sep 12, 2016

@Kureev

This comment has been minimized.

Copy link
Collaborator

commented Sep 13, 2016

@Kureev

This comment has been minimized.

Copy link
Collaborator

commented Sep 13, 2016

Could you be so kind to take a look why import fails, @bestander?

@ghost ghost added the Import Started label Sep 13, 2016

@ghost

This comment has been minimized.

Copy link

commented Sep 13, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

@ghost ghost added the CLA Signed label Sep 13, 2016

@bestander

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

Will do

@ghost

This comment has been minimized.

Copy link

commented Sep 14, 2016

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@ghost ghost added CLA Signed and removed Import Started labels Sep 14, 2016

@bestander

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2016

This file was changed in #9819.
Could you rebase?

[local-cli] Resolve symlinks recursively
- Throw an error for circular symlinks
- Rename (and simplify) `isSubPathOfPath` to `isDescendant`
- Rename `isSubPathOfPaths` to `rootExists`
- Rename var `existingSearchPaths` to `ignoredRoots`
- Squash symlink resolution into a single `forEach`
- Pass both `args.projectRoots` and `args.root` to `findSymlinksPaths`

@aleclarson aleclarson force-pushed the aleclarson:symlinks-to-symlinks branch from 165f1ba to 817f7f8 Sep 14, 2016

@ghost

This comment has been minimized.

Copy link

commented Sep 14, 2016

@aleclarson updated the pull request - view changes - changes since last import

1 similar comment
@ghost

This comment has been minimized.

Copy link

commented Sep 14, 2016

@aleclarson updated the pull request - view changes - changes since last import

@Kureev

Kureev approved these changes Sep 15, 2016

Copy link
Collaborator

left a comment

LGTM! 👍

@Kureev

This comment has been minimized.

Copy link
Collaborator

commented Sep 15, 2016

@ghost

This comment has been minimized.

Copy link

commented Sep 15, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

@ghost ghost added the CLA Signed label Sep 15, 2016

@ghost ghost closed this in fa6191f Sep 15, 2016

@ghost ghost added the CLA Signed label Sep 15, 2016

@aleclarson aleclarson deleted the aleclarson:symlinks-to-symlinks branch Sep 16, 2016

@ekryski

This comment has been minimized.

Copy link

commented Sep 22, 2016

@aleclarson nice work! Sorry I was MIA for a bit on my other PR but then this came along 😄 . Stoked!

@ericvicenti

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2016

I see this is closed, but symlinks are still not working for me. Did we have a regression?

grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018

Support symlinks-to-symlinks
Summary:
In response to [this comment](facebook/react-native#9009 (comment)).

I could be wrong here, but I think Watchman can't handle symlinks, so we need to make sure symlinks-to-symlinks are handled up front.
Closes facebook/react-native#9792

Differential Revision: D3858349

Pulled By: bestander

fbshipit-source-id: f3a34dae90ed9a7004a03158288db5e1932bfc69

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.