$ENV{HOME}/.cpanm does not work well on MS Windows #132

Closed
kmx opened this Issue Nov 15, 2011 · 21 comments

Comments

Projects
None yet
4 participants
@kmx

kmx commented Nov 15, 2011

HOME env variable does not exist on MS Windows by default therefore $ENV{HOME}/.cpanm usually ended up to be C:.cpanm

On MS Windows there are:
HOMEDRIVE=C:
HOMEPATH=\Users\loginname

IMHO using File::HomeDir would be the best way

--kmx

@kmx

This comment has been minimized.

Show comment
Hide comment
@kmx

kmx Nov 15, 2011

The patch might look like this:

        bless {
-           home => "$ENV{HOME}/.cpanm",
+           home => (eval {require File::HomeDir} ? File::HomeDir->my_home : $ENV{HOME}) . "/.cpanm",
            cmd  => 'install',

kmx commented Nov 15, 2011

The patch might look like this:

        bless {
-           home => "$ENV{HOME}/.cpanm",
+           home => (eval {require File::HomeDir} ? File::HomeDir->my_home : $ENV{HOME}) . "/.cpanm",
            cmd  => 'install',
@kmx

This comment has been minimized.

Show comment
Hide comment
@kmx

kmx Apr 4, 2012

Could you please give me some feedback my patch proposed in this RT?

Thanks

kmx commented Apr 4, 2012

Could you please give me some feedback my patch proposed in this RT?

Thanks

@shadowcat-mst

This comment has been minimized.

Show comment
Hide comment
@shadowcat-mst

shadowcat-mst Aug 11, 2012

Contributor

@miyagawa what's currently blocking this?

Contributor

shadowcat-mst commented Aug 11, 2012

@miyagawa what's currently blocking this?

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Aug 11, 2012

Owner

File::HomeDir creates a bad path on Mac OS X and I don't want to make cpanm rely on that.

Owner

miyagawa commented Aug 11, 2012

File::HomeDir creates a bad path on Mac OS X and I don't want to make cpanm rely on that.

@wchristian

This comment has been minimized.

Show comment
Hide comment
@wchristian

wchristian Aug 12, 2012

Contributor

I want to fix that. Can you please describe the issue in more detail? :)

Contributor

wchristian commented Aug 12, 2012

I want to fix that. Can you please describe the issue in more detail? :)

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Aug 12, 2012

Owner

Some versions of the module returns $HOME/Library/Application Support which is completely bogus and once broke CPAN.pm config, hence I don't trust the module.

Owner

miyagawa commented Aug 12, 2012

Some versions of the module returns $HOME/Library/Application Support which is completely bogus and once broke CPAN.pm config, hence I don't trust the module.

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Aug 12, 2012

Owner

It also has weird dependencies on Carbon and Cocoa API in runtime, which is far more complicated and unreliable than just using $HOME.

Owner

miyagawa commented Aug 12, 2012

It also has weird dependencies on Carbon and Cocoa API in runtime, which is far more complicated and unreliable than just using $HOME.

@wchristian

This comment has been minimized.

Show comment
Hide comment
@wchristian

wchristian Aug 12, 2012

Contributor

I think this can be fixed.

I can try and give adam a patch to add File::HomeDir::PP, which would use ONLY pureperl code to determine the homedir.

Would that be enough for you to use it?

Contributor

wchristian commented Aug 12, 2012

I think this can be fixed.

I can try and give adam a patch to add File::HomeDir::PP, which would use ONLY pureperl code to determine the homedir.

Would that be enough for you to use it?

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Aug 12, 2012

Owner

My whole point is we should just use $HOME when it's available because it's much simpler and has no complicated dependencies.

I could accept to fall back to File::HomeDir in environments where it is not available, but that makes cpanm non-bootstrappable unless it's fatpacked, which worries me a little.

Owner

miyagawa commented Aug 12, 2012

My whole point is we should just use $HOME when it's available because it's much simpler and has no complicated dependencies.

I could accept to fall back to File::HomeDir in environments where it is not available, but that makes cpanm non-bootstrappable unless it's fatpacked, which worries me a little.

@wchristian

This comment has been minimized.

Show comment
Hide comment
@wchristian

wchristian Aug 12, 2012

Contributor

Alright, so probably make File::HomeDir::PP a separate dist, move most of the core of File::HomeDir in there and make File::HomeDir the dist that can use fancier stuff.

In the meantime, your second sentence there gave me an idea. How do you feel about:

$ENV{HOME} || do { require File::HomeDir; File::HomeDir->my_home } 
Contributor

wchristian commented Aug 12, 2012

Alright, so probably make File::HomeDir::PP a separate dist, move most of the core of File::HomeDir in there and make File::HomeDir the dist that can use fancier stuff.

In the meantime, your second sentence there gave me an idea. How do you feel about:

$ENV{HOME} || do { require File::HomeDir; File::HomeDir->my_home } 
@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Aug 12, 2012

Owner

Yep, that would be IMO much better, although it gives a runtime error when you don't have File::HomeDir, which sounds like worse than using C:\.cpanm as a working directory?

Owner

miyagawa commented Aug 12, 2012

Yep, that would be IMO much better, although it gives a runtime error when you don't have File::HomeDir, which sounds like worse than using C:\.cpanm as a working directory?

@shadowcat-mst

This comment has been minimized.

Show comment
Hide comment
@shadowcat-mst

shadowcat-mst Aug 12, 2012

Contributor

What about logic like:

$ENV{HOME} || eval { require File::HomeDir; File::HomeDir->my_home } || join('', @ENV{qw(HOMEDRIVE HOMEPATH)})

given we can basically assume $ENV{HOME} on *n?x anyway?

(also, yes, File::HomeDir got it wrong once ... but since Adam's been a lot more careful since, unless you're willing to fork it I really don't see the gain in avoiding it just because it once had a bug)

Contributor

shadowcat-mst commented Aug 12, 2012

What about logic like:

$ENV{HOME} || eval { require File::HomeDir; File::HomeDir->my_home } || join('', @ENV{qw(HOMEDRIVE HOMEPATH)})

given we can basically assume $ENV{HOME} on *n?x anyway?

(also, yes, File::HomeDir got it wrong once ... but since Adam's been a lot more careful since, unless you're willing to fork it I really don't see the gain in avoiding it just because it once had a bug)

@wchristian

This comment has been minimized.

Show comment
Hide comment
@wchristian

wchristian Aug 12, 2012

Contributor

it gives a runtime error when you don't have File::HomeDir, which sounds like worse than using C:.cpanm as a working directory?

It's honestly not worse. On unix systems you're guaranteed to have a $HOME, so this doesn't happen there, but if $HOME is undefined you get warnings for one, and for the other, straight-up undefined behavior:

  • it won't always go to C:, it will go to whatever partition $CWD is currently on (which can change both before and during runtime)
  • if you actually do have multiple users on a windows machine (easy to happen due to users for apps, etc.) they'll try to use the same dir
  • if it actually DOES use C:, then that's the same as if it had used /.cpanm on a unix system

So, yeah, i'd prefer it crash.

But honestly, it's bike-shedding and i'll be happy to use either do or eval if there is a fallback as in mst's proposed solution. Having said that, which would you prefer? Once i know i'll make the appropiate pull request.

Contributor

wchristian commented Aug 12, 2012

it gives a runtime error when you don't have File::HomeDir, which sounds like worse than using C:.cpanm as a working directory?

It's honestly not worse. On unix systems you're guaranteed to have a $HOME, so this doesn't happen there, but if $HOME is undefined you get warnings for one, and for the other, straight-up undefined behavior:

  • it won't always go to C:, it will go to whatever partition $CWD is currently on (which can change both before and during runtime)
  • if you actually do have multiple users on a windows machine (easy to happen due to users for apps, etc.) they'll try to use the same dir
  • if it actually DOES use C:, then that's the same as if it had used /.cpanm on a unix system

So, yeah, i'd prefer it crash.

But honestly, it's bike-shedding and i'll be happy to use either do or eval if there is a fallback as in mst's proposed solution. Having said that, which would you prefer? Once i know i'll make the appropiate pull request.

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Aug 12, 2012

Owner

if it actually DOES use C:, then that's the same as if it had used /.cpanm on a unix system

Well not really, because on UNIX it will most likely fail with a permission error while Win32 doesn't.

So, yeah, i'd prefer it crash.

I wouldn't, since File::HomeDir is not a core module, cpanm would become non-bootstrappable (unless we fatpack it).

$ENV{HOME} || eval { require File::HomeDir; File::HomeDir->my_home } || join('', @ENV{qw(HOMEDRIVE HOMEPATH)})

I like this. Update or resubmit the pull request with this and I will get it merged.

Thanks.

Owner

miyagawa commented Aug 12, 2012

if it actually DOES use C:, then that's the same as if it had used /.cpanm on a unix system

Well not really, because on UNIX it will most likely fail with a permission error while Win32 doesn't.

So, yeah, i'd prefer it crash.

I wouldn't, since File::HomeDir is not a core module, cpanm would become non-bootstrappable (unless we fatpack it).

$ENV{HOME} || eval { require File::HomeDir; File::HomeDir->my_home } || join('', @ENV{qw(HOMEDRIVE HOMEPATH)})

I like this. Update or resubmit the pull request with this and I will get it merged.

Thanks.

@kmx

This comment has been minimized.

Show comment
Hide comment
@kmx

kmx Aug 18, 2012

Hi,

there is one more issue on Windows XP - spaces in dirname (unfortunately I am using Windows 7 for my testing so I have not found this earlier).

Would you agree with patch like this:

   sub new {
       my $class = shift;

+      my $home = $ENV{HOME} || eval { require File::HomeDir; File::HomeDir->my_home } || join('', @ENV{qw(HOMEDRIVE HOMEPATH)});
+      if (WIN32) {
+        require Win32;
+        $home = Win32::GetShortPathName($home); #avoid spaces in dirname
+      }
+
       bless {
-          home => "$ENV{HOME}/.cpanm",
+          home => "$home/.cpanm",
           cmd  => 'install',

If yes I will resubmit the pull request.

kmx

kmx commented Aug 18, 2012

Hi,

there is one more issue on Windows XP - spaces in dirname (unfortunately I am using Windows 7 for my testing so I have not found this earlier).

Would you agree with patch like this:

   sub new {
       my $class = shift;

+      my $home = $ENV{HOME} || eval { require File::HomeDir; File::HomeDir->my_home } || join('', @ENV{qw(HOMEDRIVE HOMEPATH)});
+      if (WIN32) {
+        require Win32;
+        $home = Win32::GetShortPathName($home); #avoid spaces in dirname
+      }
+
       bless {
-          home => "$ENV{HOME}/.cpanm",
+          home => "$home/.cpanm",
           cmd  => 'install',

If yes I will resubmit the pull request.

kmx

@wchristian

This comment has been minimized.

Show comment
Hide comment
@wchristian

wchristian Aug 25, 2012

Contributor

kmx, if spaces in dir names are an issue, then i believe there is some other code that should be fixed, rather than pulling in more dependencies.

Contributor

wchristian commented Aug 25, 2012

kmx, if spaces in dir names are an issue, then i believe there is some other code that should be fixed, rather than pulling in more dependencies.

@kmx

This comment has been minimized.

Show comment
Hide comment
@kmx

kmx Aug 25, 2012

If you mean "require Win32" then it is not such a big trouble as Win32 is part of perl core (of course if you are on MS Win)

kmx commented Aug 25, 2012

If you mean "require Win32" then it is not such a big trouble as Win32 is part of perl core (of course if you are on MS Win)

@kmx

This comment has been minimized.

Show comment
Hide comment
@kmx

kmx Oct 1, 2012

please have a look at #172

kmx commented Oct 1, 2012

please have a look at #172

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Oct 1, 2012

Owner

+1 on what Christian said. Should probably fix the code to work with $home including a whitespace rather than escaping it earlier.

On Oct 1, 2012, at 7:22 PM, kmx wrote:

please have a look at miyagawa/cpanminus#172


Reply to this email directly or view it on GitHub.

Owner

miyagawa commented Oct 1, 2012

+1 on what Christian said. Should probably fix the code to work with $home including a whitespace rather than escaping it earlier.

On Oct 1, 2012, at 7:22 PM, kmx wrote:

please have a look at miyagawa/cpanminus#172


Reply to this email directly or view it on GitHub.

@kmx

This comment has been minimized.

Show comment
Hide comment
@kmx

kmx Oct 2, 2012

GetShortPathName is not about escaping it is about getting an alternative name which is kind of guaranteed to be space-less

You can remove GetShortPathName part, however it means troubles for Windows XP users (as their home is in 'C:\Documents and Setting\Username')

kmx commented Oct 2, 2012

GetShortPathName is not about escaping it is about getting an alternative name which is kind of guaranteed to be space-less

You can remove GetShortPathName part, however it means troubles for Windows XP users (as their home is in 'C:\Documents and Setting\Username')

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Feb 1, 2013

Owner

merged at 2e375ab

Owner

miyagawa commented Feb 1, 2013

merged at 2e375ab

@miyagawa miyagawa closed this Feb 1, 2013

miyagawa added a commit that referenced this issue Feb 1, 2013

Checking in changes prior to tagging of version 1.59_03.
Changelog diff is:

diff --git a/Changes b/Changes
index 081828b..d6f7dfe 100644
--- a/Changes
+++ b/Changes
@@ -1,5 +1,11 @@
 See http://github.com/miyagawa/cpanminus/ for the latest development.

+1.59_03 Fri Feb  1 10:42:57 PST 2013
+   [Improvements]
+      - Fix issues working with file:// URLs with drive letters on Win32 (A.J. Lucas) #180
+      - Fix home directory detection without HOME env on win32 (kmx, Christian Walde) #132
+      - Allow comment fields in 02packages file (Jeffrey Thalhammer) #187
+
 1.59_02 Thu Jan 31 19:09:43 PST 2013
    [New Features]
       - Added experimental @ shortcut to mean exact version e.g. cpanm DBI@1.20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment