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

Nested executables fix #2053

Closed
wants to merge 3 commits into from
Closed

Nested executables fix #2053

wants to merge 3 commits into from

Conversation

onemen
Copy link

@onemen onemen commented Nov 28, 2016

Implementing 'Nested executables fix' as suggested by #1210

@torifat
Copy link
Member

torifat commented Nov 29, 2016

@onemen I have re-ran you test. It's passing now 👍

This is quite a big PR. Might take some time to review.

shanewilson added a commit to knitjs/knit that referenced this pull request Nov 29, 2016
yarn doesnt hoist nested bins as of now. probably fine to make that peer anyway?
yarnpkg/yarn#2053
yarnpkg/yarn#1210
@bestander
Copy link
Member

@onemen, thanks for the effort to add all those tests! 👍
Could you rebase please?

@onemen
Copy link
Author

onemen commented Dec 4, 2016

@bestander,

I will do the rebase later this week.
I want to add test, to make sure that the bins files are actually usable.
That every module can find the bins of its dependency.

Where can i found simple example for this?

Implementing 'Nested executables fix' as suggested by #1210
* Use cwd instead of package directory when linking bin scripts
* Update package-linker module to filter calls to linkBin function
(prevents #1823)
* Update global module to only link root packages to the global bin
folder
* Added tests
Copy link
Member

@bestander bestander left a comment

Choose a reason for hiding this comment

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

Great work on tests!
Just need to make sure that we deal with conflicts correctly

const binLoc = path.dirname(location) === pkgLoc ? parentBinLoc : pkgBinLoc;
const binLinks = await getBinLinks(binLoc);
// Remove Duplicates
if (!binLinks.has(location)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think top level dependencies should have a priority over nested ones

Copy link
Author

Choose a reason for hiding this comment

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

I can fix the sort at the top of copyModules function, let me know if it ok with you.
After this sort, we populate binLinks with top level dependencies first.

    // sorted tree makes file creation and copying not to interfere with each other
+   // prioritize top level dependencies
    flatTree = flatTree.sort(function(dep1, dep2): number {
-     return dep1[0].localeCompare(dep2[0]);
+     const [len1, len2] = [dep1[0].split(path.sep).length, dep2[0].split(path.sep).length];
+     return len1 === len2 ? dep1[0].localeCompare(dep2[0]) : len1 - len2;
    });

Copy link
Member

Choose a reason for hiding this comment

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

If that works then sure.
I thought it would be easier though if you iterate over the top dependencies first to generate a set of bins and then go through deeper dependencies discarding bins that are already in the set.

});
await install.init();
await check(config, reporter, {}, []);
return install;
Copy link
Member

Choose a reason for hiding this comment

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

this test needs to verify that bin dependencies were flattened properly.
Especially for a case of conflicting bins

@skevy
Copy link
Contributor

skevy commented Feb 1, 2017

@onemen any updates on finishing up this PR?

@onemen
Copy link
Author

onemen commented Feb 1, 2017

I hope to find the time later this month.

I need to re-base the code and create new branch, since my old branch was corrupted

@thomasruiz
Copy link

@onemen any update?

@bestander
Copy link
Member

ping.
Would be great to get this in, otherwise let's close the PR.

@bestander
Copy link
Member

I'll close this PR for now.
Feel free to send a new one.

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.

None yet

5 participants