search

mattsawyer77 / atom-perforce

Perforce integration for the Atom editor
MIT License
18 stars 12 forks source link

Fixes #34 #38

Closed mdouglass closed 9 years ago

mdouglass commented 9 years ago

Include the HOME variable in the extracted environment variables

mdouglass commented 9 years ago

Yep, absolutely. This is my understanding of what's going on.

The atom-perforce plugin at startup extracts a bunch of environment variables from shell config files via extractVarsFromEnvironment. On my system, with the previous code, extractVarsFromEnvironment results in an empty object { }. With the changed code, it results in an object that looks like `{ HOME: '/Users/mdouglass' }.

atom-perforce.js:31-33 takes that object (acccessed via the environmentReady promise) and passes it to node-perforce as the env parameter of the options hash. node-perforce internally calls child_process.exec passing that as the environment to use in the shell. Passing an empty object here results in p4 running with an empty environment.

The p4 executable in turn uses the HOME environment variable to find the .p4tickets file which contains the current session info for the user. In the old code, there was no HOME set and so it would fail with the invalid password error. With the new code, HOME is properly imported, p4 can find the .p4tickets file and everything works (at least for me).

Hope that explains it well enough.

mdouglass commented 9 years ago

Note: I can verify the HOME environment variable missing causing the same problem at the command line:

SFOC02KT0LUFFT1:kmservices mdouglass$ /bin/sh -c 'p4 diff main.js'
main.js - file(s) not opened on this client.
SFOC02KT0LUFFT1:kmservices mdouglass$ HOME= /bin/sh -c 'p4 diff main.js'
Perforce password (P4PASSWD) invalid or unset.
unional commented 9 years ago

Just curious, do you know why it works prior to 1.6?

mdouglass commented 9 years ago

I haven't tried to debug it to be certain, but prior to https://github.com/mattsawyer77/atom-perforce/commit/9482b650046aa98a77329ee10a91a932fb7ab087 the execP4Command function did not exist. And the individual calls to node_perforce did not set env in their options hash.

To be fair, in code before that commit, I can't see how any environment variables other than PATH are ever used. It looks to me like they get returned from setupEnvironment to activate but are then swallowed and ignore.

Strike that - in the previous code, it looks like the environments library directly grabs the listed environment variables and pushes them into process.env. The net result there is that HOME was left unchanged in process.env and therefore node_perforce inherited whatever value Atom had for that already.