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

Fix global add on Windows #1043

Merged
merged 4 commits into from
Jan 3, 2017
Merged

Conversation

FLGMwt
Copy link
Contributor

@FLGMwt FLGMwt commented Oct 14, 2016

Summary
yarn global add <package> is almost entirely broken on Windows due to the global linking workflow trying to use cmd-shim for linking more than one level. 😞

During a global install, we first do the normal install, which cmd-shims all the bins into node_modules/.bin. cmd-shiming involves creating a sh style and cmd style passthru script for each bin in the package. However, since yarn globals link to those and not the package bins directly (like npm does), we need and extra level of linking from globalbin to node_modules/.bin.

node_modules/.bin/pm2 turns into node/pm2 and node/pm2.cmd
then
node_modules/.bin/pm2.cmd turns into node/pm2.cmd and node/pm2.cmd.cmd

The second node/pm2.cmd overwrites the first, but there's no way to configure cmd-shim to do anything else. Whats more, neither of the two pm2.cmds are valid. The first one is a cmd-style wrapper to a sh style script and the second (despite the name) is a sh-style script to a cmd style script in the /.bin.

The two valid files are node/pm2 which is an sh that calls the .bin/pm2 sh which calls the package bin and node/pm2.cmd.cmd which is a cmd that calls the .bin/pm2.cmd which calls the package bin. Without any way to configure cmd-shim, I opted for renaming node/pm2.cmd.cmd as node/pm2.cmd after the shims are done.

I've been struggling for a while about how to fix this (might involve a forking cmd-shim), but I think it's worth temporarily shifting the jank from global installs not working at all to not working exactly as they should.

Test plan
Before: yarn global add pm2 resulted in a broken global install and an extra file pm2.cmd.cmd.

After changes, pm2 works correctly after installing in powershell shown here:

> yarn global add pm2
yarn global v0.15.1
warning No license field
...
success Installed pm2@2.0.18 with binaries:
      - pm2
      - rundev
      - pm2-dev
      - pm2-docker
Done in 8.26s.
>
> pm2 -v
2.0.18

and the global bin has only the sh and cmd files:

> ls pm2*
    Directory: C:\Program Files\nodejs
Mode                LastWriteTime         Length Name
----                -------------         ------ ----
-a----       10/14/2016  12:06 AM            420 pm2
-a----       10/14/2016  12:06 AM            428 pm2-dev
-a----       10/14/2016  12:06 AM             97 pm2-dev.cmd
-a----       10/14/2016  12:06 AM            434 pm2-docker
-a----       10/14/2016  12:06 AM            100 pm2-docker.cmd
-a----       10/14/2016  12:06 AM             93 pm2.cmd

@FLGMwt
Copy link
Contributor Author

FLGMwt commented Oct 14, 2016

Fixes #927 and #958

@FLGMwt FLGMwt mentioned this pull request Oct 14, 2016
@FLGMwt FLGMwt changed the title rename cmd link to cmd shim from .cmd.cmd to .cmd Fix global add on Windows Oct 14, 2016
@onemen
Copy link

onemen commented Oct 21, 2016

What do you think about #1226 ?

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.

Makes sense.
Could you fix a nit and let's merge it

@@ -114,6 +114,9 @@ async function initUpdateBins(config: Config, reporter: Reporter): Promise<() =>
const dest = path.join(binFolder, path.basename(src));
await fs.unlink(dest);
await linkBin(src, dest);
if (process.platform === 'win32' && dest.indexOf('.cmd') != -1) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: !== -1

@bestander bestander self-assigned this Oct 28, 2016
@darahak
Copy link

darahak commented Dec 12, 2016

@FLGMwt Are you still working on it? Looks like it's so close to being merged.

@leohxj
Copy link

leohxj commented Dec 14, 2016

merged? I am using 0.17.10 on Windows7, it still create .cmd.cmd file.

@FLGMwt
Copy link
Contributor Author

FLGMwt commented Dec 29, 2016

Sorry all! Lost my GitHub notifications for awhile and missed the response.

Nit fixed @bestander : )

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