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

It doesn't work on Windows #46

Closed
PhiLhoSoft opened this Issue Dec 16, 2015 · 13 comments

Comments

Projects
None yet
3 participants
@PhiLhoSoft

PhiLhoSoft commented Dec 16, 2015

OK, title is a bit provocative, I should add "out of the box"...

First, thanks for the plugin: we use Perforce at work, and hand-checking out these pesky read-only files would be a pain... I used Brackets previously, it has a similar plugin (but more primitive), it is very useful (although intermittently failing, probably because P4 doesn't answer fast enough).

I use Atom 1.3.1 on Win7.
P4 is in the path.
It is located at: C:\Program Files (x86)\Perforce\p4.EXE

First, I see in the settings of your package:
p4
You probably wanted to put C:\Program Files\Perforce but the backslashes were lost...
But it is supposed to pick p4 from path, alas it doesn't do it.

I pasted the above path (without p4.exe), but it still doesn't work.
I changed the backslashes to forward slashes, without success.
Tried with double quotes around the path, no luck.

Looked at console, found the following:

Error: spawn C:\Windows\system32\cmd.exe ENOENT(…) -- C:\Users\plhoste.atom\packages\atom-perforce\lib\atom-perforce.js:570
Error: spawn C:\Windows\system32\cmd.exe ENOENT(…) -- C:\Users\plhoste.atom\packages\atom-perforce\lib\atom-perforce.js:611
Error: spawn C:\Windows\system32\cmd.exe ENOENT(…)
C:\Sources\ea\designer\service\service-designer\src\main\webapp\app\components\templateManagement is outside any known perforce workspace -- C:\Users\plhoste.atom\packages\atom-perforce\lib\atom-perforce.js:187
Error: spawn C:\Windows\system32\cmd.exe ENOENT(…)

Note: the file IS in a Perforce workspace, of course.
I get the message when I manually do Packages > Perforce > Edit.

Atom shows the following message when I save with auto-edit activated:

Unable to save file 'C:\Sources\someProject\src\main\webapp\app\components\templateManagement\templateCreationController.js'
EPERM: operation not permitted, open 'C:\Sources\someProject\src\main\webapp\app\components\templateManagement\templateCreationController.js'

OK, I will take a look at the source code, but I promise nothing: I am not a Node.js dev, neither an Atom one!

@PhiLhoSoft

This comment has been minimized.

Show comment
Hide comment
@PhiLhoSoft

PhiLhoSoft Dec 16, 2015

settings.js: defaultWindowsP4Directory = 'C:\Program Files\Perforce'
These backslashes are not valid! Must be doubled, \P isn't a valid escape...

PhiLhoSoft commented Dec 16, 2015

settings.js: defaultWindowsP4Directory = 'C:\Program Files\Perforce'
These backslashes are not valid! Must be doubled, \P isn't a valid escape...

@PhiLhoSoft

This comment has been minimized.

Show comment
Hide comment
@PhiLhoSoft

PhiLhoSoft Dec 16, 2015

OK, found an issue, line 169 of atom-perforce.js
p4Info.currentDirectory.toLowerCase().startsWith(p4Info.clientRoot.toLowerCase())
p4Info.currentDirectory is "c:\Sources\someProject\somePath\etc"
p4Info.clientRoot is "C:/Sources"
Note that my P4ROOT environment variable is C:\Sources...

The paths should be normalized at some point (and you should pull out this test to its own function, I see it in all commands...).

PhiLhoSoft commented Dec 16, 2015

OK, found an issue, line 169 of atom-perforce.js
p4Info.currentDirectory.toLowerCase().startsWith(p4Info.clientRoot.toLowerCase())
p4Info.currentDirectory is "c:\Sources\someProject\somePath\etc"
p4Info.clientRoot is "C:/Sources"
Note that my P4ROOT environment variable is C:\Sources...

The paths should be normalized at some point (and you should pull out this test to its own function, I see it in all commands...).

@unional

This comment has been minimized.

Show comment
Hide comment
@unional

unional Dec 16, 2015

Collaborator

Let me take a look into it. I introduced that code.

But why would you have C:/Source...that's not normal in Windows.

Collaborator

unional commented Dec 16, 2015

Let me take a look into it. I introduced that code.

But why would you have C:/Source...that's not normal in Windows.

@unional

This comment has been minimized.

Show comment
Hide comment
@unional

unional Dec 16, 2015

Collaborator

@mattsawyer77 , do you mind if I add testing using mocha and chai in your project?

Collaborator

unional commented Dec 16, 2015

@mattsawyer77 , do you mind if I add testing using mocha and chai in your project?

@unional

This comment has been minimized.

Show comment
Hide comment
@unional

unional Dec 16, 2015

Collaborator

FYI, my p4 on windows server 2012 R2 is installed at C:\Program Files\Perforce\p4.exe. So just checking platform seems to be not sufficient.
Considering where p4 on windows.

Collaborator

unional commented Dec 16, 2015

FYI, my p4 on windows server 2012 R2 is installed at C:\Program Files\Perforce\p4.exe. So just checking platform seems to be not sufficient.
Considering where p4 on windows.

@mattsawyer77

This comment has been minimized.

Show comment
Hide comment
@mattsawyer77

mattsawyer77 Dec 16, 2015

Owner

@unional that would be awesome. thanks for all your help!

Owner

mattsawyer77 commented Dec 16, 2015

@unional that would be awesome. thanks for all your help!

@unional

This comment has been minimized.

Show comment
Hide comment
@unional

unional Dec 16, 2015

Collaborator

@PhiLhoSoft , it should work when @mattsawyer77 merge #49 . I originally didn't pull it into its own function because I don't want to introduce extra changes. But now it is . :)

Collaborator

unional commented Dec 16, 2015

@PhiLhoSoft , it should work when @mattsawyer77 merge #49 . I originally didn't pull it into its own function because I don't want to introduce extra changes. But now it is . :)

@PhiLhoSoft

This comment has been minimized.

Show comment
Hide comment
@PhiLhoSoft

PhiLhoSoft Dec 17, 2015

I made some changes to see if I can make it to work... At least, it was interesting to see I can easily hack and debug an Atom package... 😄

I can confirm it works after normailizing the slashes. I still see an error at start on p4 info, I will investigate. (Or perhaps I got it because I wasn't logged in yet in Perforce? Will check.)

function normalizePath(path) {
    return path.toLowerCase().replace(/\\/g, '/');
}

function checkIsInWorkspace(p4Info) {
    return !p4Info['clientUnknown.'] && normalizePath(p4Info.currentDirectory).startsWith(normalizePath(p4Info.clientRoot));
}

and called checkIsInWorkspace(p4Info) everywhere it is used (there is a reversed condition, I just ! it...).

But why would you have C:/Source...that's not normal in Windows.

Well, this style of path works in Windows in 99 % of cases, so it is safer to rely on it...

Considering where p4 on windows.

Would work on my computer, as I installed Unix utilities (UnxUtils or GnuWin32), but it won't work, AFAIK, on most Windows computers.

Note: after my hack, the package works with p4 in the path (ie. I removed the path from the settings), so just keep the current behavior as is (just fix these pseudo escape keys in the string! 😄). It is highly probable to find p4 in the path anyway.

PhiLhoSoft commented Dec 17, 2015

I made some changes to see if I can make it to work... At least, it was interesting to see I can easily hack and debug an Atom package... 😄

I can confirm it works after normailizing the slashes. I still see an error at start on p4 info, I will investigate. (Or perhaps I got it because I wasn't logged in yet in Perforce? Will check.)

function normalizePath(path) {
    return path.toLowerCase().replace(/\\/g, '/');
}

function checkIsInWorkspace(p4Info) {
    return !p4Info['clientUnknown.'] && normalizePath(p4Info.currentDirectory).startsWith(normalizePath(p4Info.clientRoot));
}

and called checkIsInWorkspace(p4Info) everywhere it is used (there is a reversed condition, I just ! it...).

But why would you have C:/Source...that's not normal in Windows.

Well, this style of path works in Windows in 99 % of cases, so it is safer to rely on it...

Considering where p4 on windows.

Would work on my computer, as I installed Unix utilities (UnxUtils or GnuWin32), but it won't work, AFAIK, on most Windows computers.

Note: after my hack, the package works with p4 in the path (ie. I removed the path from the settings), so just keep the current behavior as is (just fix these pseudo escape keys in the string! 😄). It is highly probable to find p4 in the path anyway.

@unional

This comment has been minimized.

Show comment
Hide comment
@unional

unional Dec 17, 2015

Collaborator

Oh, didn't know where p4 might not work. Will test it on another system
tomorrow. Thanks!
On Thu, Dec 17, 2015 at 12:51 AM Philippe Lhoste notifications@github.com
wrote:

I made some changes to see if I can make it to work... At least, it was
interesting to see I can easily hack and debug an Atom package... [image:
😄]

I can confirm it works after normailizing the slashes. I still see an
error at start on p4 info, I will investigate. (Or perhaps I got it
because I wasn't logged in yet in Perforce? Will check.)

function normalizePath(path) {
return path.toLowerCase().replace(//g, '/');
}

function checkIsInWorkspace(p4Info) {
return !p4Info['clientUnknown.'] && normalizePath(p4Info.currentDirectory).startsWith(normalizePath(p4Info.clientRoot));
}

and called checkIsInWorkspace(p4Info) everywhere it is used (there is a
reversed condition, I just ! it...).

But why would you have C:/Source...that's not normal in Windows.

Well, this style of path works in Windows in 99 % of cases, so it is safer
to rely on it...

Considering where p4 on windows.

Would work on my computer, as I installed Unix utilities (UnxUtils or
GnuWin32), but it won't work, AFAIK, on most Windows computers.

Note: after my hack, the package works with p4 in the path (ie. I removed
the path from the settings), so just keep the current behavior as is (just
fix these pseudo escape keys in the string! [image: 😄]).


Reply to this email directly or view it on GitHub
#46 (comment)
.

Collaborator

unional commented Dec 17, 2015

Oh, didn't know where p4 might not work. Will test it on another system
tomorrow. Thanks!
On Thu, Dec 17, 2015 at 12:51 AM Philippe Lhoste notifications@github.com
wrote:

I made some changes to see if I can make it to work... At least, it was
interesting to see I can easily hack and debug an Atom package... [image:
😄]

I can confirm it works after normailizing the slashes. I still see an
error at start on p4 info, I will investigate. (Or perhaps I got it
because I wasn't logged in yet in Perforce? Will check.)

function normalizePath(path) {
return path.toLowerCase().replace(//g, '/');
}

function checkIsInWorkspace(p4Info) {
return !p4Info['clientUnknown.'] && normalizePath(p4Info.currentDirectory).startsWith(normalizePath(p4Info.clientRoot));
}

and called checkIsInWorkspace(p4Info) everywhere it is used (there is a
reversed condition, I just ! it...).

But why would you have C:/Source...that's not normal in Windows.

Well, this style of path works in Windows in 99 % of cases, so it is safer
to rely on it...

Considering where p4 on windows.

Would work on my computer, as I installed Unix utilities (UnxUtils or
GnuWin32), but it won't work, AFAIK, on most Windows computers.

Note: after my hack, the package works with p4 in the path (ie. I removed
the path from the settings), so just keep the current behavior as is (just
fix these pseudo escape keys in the string! [image: 😄]).


Reply to this email directly or view it on GitHub
#46 (comment)
.

@PhiLhoSoft

This comment has been minimized.

Show comment
Hide comment
@PhiLhoSoft

PhiLhoSoft Dec 17, 2015

Errors on startup: I get them on the getOpenedFiles call, and on the showClientName one.
Both fail because they call p4 info on a path that isn't legal: atom://about.
I think I get the latter because Atom display an About page on startup, and I haven't figured out yet how to get rid of it, but it should be handled anyway.
Perhaps with a test if the path starts with atom: to avoid this special scheme...

Note: I also suggest to avoid doing a console.log() of results on the getChanges call: no need to spam the console with lengthly results...

PhiLhoSoft commented Dec 17, 2015

Errors on startup: I get them on the getOpenedFiles call, and on the showClientName one.
Both fail because they call p4 info on a path that isn't legal: atom://about.
I think I get the latter because Atom display an About page on startup, and I haven't figured out yet how to get rid of it, but it should be handled anyway.
Perhaps with a test if the path starts with atom: to avoid this special scheme...

Note: I also suggest to avoid doing a console.log() of results on the getChanges call: no need to spam the console with lengthly results...

@PhiLhoSoft

This comment has been minimized.

Show comment
Hide comment
@PhiLhoSoft

PhiLhoSoft Dec 17, 2015

OK, i no longer have errors in the console, and I can check out files.
I am a bit in a hurry, and won't have time to do a real PR, so I just pasted the file with my changes at https://gist.github.com/PhiLhoSoft/091f05e78e6df3472096

Just take inspiration from these changes, perhaps, improve them (they are a bit hasty, etc.). Feel free to use at will.

PhiLhoSoft commented Dec 17, 2015

OK, i no longer have errors in the console, and I can check out files.
I am a bit in a hurry, and won't have time to do a real PR, so I just pasted the file with my changes at https://gist.github.com/PhiLhoSoft/091f05e78e6df3472096

Just take inspiration from these changes, perhaps, improve them (they are a bit hasty, etc.). Feel free to use at will.

@unional

This comment has been minimized.

Show comment
Hide comment
@unional

unional Dec 17, 2015

Collaborator

Fix is in #50 . :)

Collaborator

unional commented Dec 17, 2015

Fix is in #50 . :)

@PhiLhoSoft

This comment has been minimized.

Show comment
Hide comment
@PhiLhoSoft

PhiLhoSoft Jan 4, 2016

I updated the package, and I can confirm it works out of the box, now. So you can close the issue.

But I suggest to remove lines like console.log(result); to avoid spamming the console. Thanks.

PhiLhoSoft commented Jan 4, 2016

I updated the package, and I can confirm it works out of the box, now. So you can close the issue.

But I suggest to remove lines like console.log(result); to avoid spamming the console. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment