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

Do not parse ls output for file listings #24

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jjatria
Contributor

jjatria commented Jun 8, 2017

If #23 is a problem with the ls command, which might not be present or might produce different output in a non-English locale, then the most obvious solution is to stop relying on ls output (which is in general a good idea).

Since AnyEvent::FTP already depends on Path::Class, which provides a children method which gives the necessary file listing, this patch replaces one with the other to solve the issue.

@plicease

This comment has been minimized.

Show comment
Hide comment
@plicease

plicease Jun 9, 2017

Owner

We need to reproduce the behavior of ls -l for a long listing (little l). This gives you the equivalent of ls -1 (one)

Owner

plicease commented Jun 9, 2017

We need to reproduce the behavior of ls -l for a long listing (little l). This gives you the equivalent of ls -1 (one)

@jjatria

This comment has been minimized.

Show comment
Hide comment
@jjatria

jjatria Jun 9, 2017

Contributor

Oh, I see the difference.

As a possible solution you suggested bundling ls from PerlPowerTools, which as far as I cna tell is a self-contained script. How would that bundling work?

Contributor

jjatria commented Jun 9, 2017

Oh, I see the difference.

As a possible solution you suggested bundling ls from PerlPowerTools, which as far as I cna tell is a self-contained script. How would that bundling work?

@plicease

This comment has been minimized.

Show comment
Hide comment
@plicease

plicease Jun 9, 2017

Owner

I did something similar once for App::RegexFileUtils. I put the scripts in a share directory:

https://github.com/plicease/App-RegexFileUtils/tree/master/share/ppt

The POD in AnyEvent::FTP should mention that the file is bundled and what the license is. Something like this.

https://github.com/plicease/App-RegexFileUtils/blob/master/lib/App/RegexFileUtils.pm#L93

Should do some basic checking that ppt version of ls doesn't introduce any new non-core deps for Perl 5.8-5.26 (I am pretty sure that it doesn't). Or maybe make them required on windows if there are.

Should use $^X to run the script.

Owner

plicease commented Jun 9, 2017

I did something similar once for App::RegexFileUtils. I put the scripts in a share directory:

https://github.com/plicease/App-RegexFileUtils/tree/master/share/ppt

The POD in AnyEvent::FTP should mention that the file is bundled and what the license is. Something like this.

https://github.com/plicease/App-RegexFileUtils/blob/master/lib/App/RegexFileUtils.pm#L93

Should do some basic checking that ppt version of ls doesn't introduce any new non-core deps for Perl 5.8-5.26 (I am pretty sure that it doesn't). Or maybe make them required on windows if there are.

Should use $^X to run the script.

@jjatria

This comment has been minimized.

Show comment
Hide comment
@jjatria

jjatria Jul 7, 2017

Contributor

Sorry for the extended silence.

I had some trouble bundling the script from the share directory because it wouldn't work while doing a local build (so it was impossible to install the module). I tried following the example you provided, but I couldn't manage to get it to work.

Then I found File::Share, which seems to do this. I pushed some changes that use that module to use the ls script from PPT, and they seem to work (I rebased from master to try and keep things a little tidier).

Is adding File::Share as a dependency a problem?

PS: I'm not sure why the AppVeyor tests are failing. Any insights?

Contributor

jjatria commented Jul 7, 2017

Sorry for the extended silence.

I had some trouble bundling the script from the share directory because it wouldn't work while doing a local build (so it was impossible to install the module). I tried following the example you provided, but I couldn't manage to get it to work.

Then I found File::Share, which seems to do this. I pushed some changes that use that module to use the ls script from PPT, and they seem to work (I rebased from master to try and keep things a little tidier).

Is adding File::Share as a dependency a problem?

PS: I'm not sure why the AppVeyor tests are failing. Any insights?

@plicease

This comment has been minimized.

Show comment
Hide comment
@plicease

plicease Jul 7, 2017

Owner

File::Share is fine. I will take a look soon. Thanks!

I have a feeling the appveyor failures have to do with cacheing the wrong DLLs. This is either my fault or appveyor being hard to use. Or both. I can test on windows myself.

Owner

plicease commented Jul 7, 2017

File::Share is fine. I will take a look soon. Thanks!

I have a feeling the appveyor failures have to do with cacheing the wrong DLLs. This is either my fault or appveyor being hard to use. Or both. I can test on windows myself.

@plicease

This comment has been minimized.

Show comment
Hide comment
@plicease

plicease Jul 7, 2017

Owner

Merged! Thanks. I made only a couple of minor tweaks. Important one though is using $^X to execute ls.pl. /usr/bin/perl may not work with the PERL5LIB or whatnot, so we want to make sure we are using the same Perl for ls.pl as we are using for the server. this will probably fix a number of the windows failures.

Owner

plicease commented Jul 7, 2017

Merged! Thanks. I made only a couple of minor tweaks. Important one though is using $^X to execute ls.pl. /usr/bin/perl may not work with the PERL5LIB or whatnot, so we want to make sure we are using the same Perl for ls.pl as we are using for the server. this will probably fix a number of the windows failures.

@plicease plicease closed this Jul 7, 2017

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