Bug #97858 for Module-CPANTS-Analyse: wrong no_symlinks test in files not in MANIFEST

Thu Aug 07 22:15:10 2014 RURBAN [...] cpan.org - Ticket created

Subject:

wrong no_symlinks test in files not in MANIFEST

no_symlinks checks for symlinks in generated files (configure && make in a src subdir) which are not part of the distribution. Perfect::Hash $ pb -S prove -b t/z_kwalitee.t t/z_kwalitee.t .. These tests should not be running unless AUTHOR_TESTING=1 and/or RELEASE_TESTING=1! t/z_kwalitee.t .. 1/? # Failed test 'no_symlinks' # at t/z_kwalitee.t line 12. # Error: This distribution includes symbolic links (symlinks). This is bad, because there are operating systems that do not handle symlinks. # Details: # The following symlinks were found: cmph-2.0/src/.libs/libcmph.so.0;cmph-2.0/src/.libs/libcmph.so;cmph-2.0/src/.libs/libcmph.la;cmph-2.0/src/.libs/libcmph.dylib;cmph-2.0/lib/libcmph.so.0;cmph-2.0/lib/libcmph.so;cmph-2.0/lib/libcmph.dylib # Remedy: Remove the symlinks from the distribution. -- Reini Urban

Fri Aug 08 01:52:11 2014 ether [...] cpan.org - Queue changed from Test-Kwalitee to Module-CPANTS-Analyse

Fri Aug 08 01:54:07 2014 ether [...] cpan.org - Correspondence added

On 2014-08-07 19:15:10, RURBAN wrote: Show quoted text

> no_symlinks checks for symlinks in generated files (configure && make > in a src subdir) > which are not part of the distribution. > > Perfect::Hash > $ pb -S prove -b t/z_kwalitee.t > t/z_kwalitee.t .. These tests should not be running unless > AUTHOR_TESTING=1 and/or RELEASE_TESTING=1! > t/z_kwalitee.t .. 1/? > # Failed test 'no_symlinks' > # at t/z_kwalitee.t line 12. > # Error: This distribution includes symbolic links (symlinks). This is > bad, because there are operating systems that do not handle symlinks. > # Details: > # The following symlinks were found: cmph- > 2.0/src/.libs/libcmph.so.0;cmph-2.0/src/.libs/libcmph.so;cmph- > 2.0/src/.libs/libcmph.la;cmph-2.0/src/.libs/libcmph.dylib;cmph- > 2.0/lib/libcmph.so.0;cmph-2.0/lib/libcmph.so;cmph- > 2.0/lib/libcmph.dylib > # Remedy: Remove the symlinks from the distribution.

I suppose the analyser would have to check against the MANIFEST file to be sure that the file actually was part of the distribution. Otherwise, you could add a skip_all if -d '.git', which would prevent the file from running in a bare repo dir (but this would only be useful if you were using Dist::Zilla, because otherwise the bare repo dir is the only place tests ever run). Sending over to the Module-CPANTS-Analyse queue for consideration; checking MANIFEST might be simple?

Fri Aug 08 01:54:07 2014 The RT System itself - Status changed from 'new' to 'open'

Fri Aug 08 02:50:35 2014 ishigaki [...] cpan.org - Correspondence added

On Fri Aug 08 14:54:07 2014, ETHER wrote: Show quoted text

> On 2014-08-07 19:15:10, RURBAN wrote:

> > no_symlinks checks for symlinks in generated files (configure && make > > in a src subdir) > > which are not part of the distribution. > > > > Perfect::Hash > > $ pb -S prove -b t/z_kwalitee.t > > t/z_kwalitee.t .. These tests should not be running unless > > AUTHOR_TESTING=1 and/or RELEASE_TESTING=1! > > t/z_kwalitee.t .. 1/? > > # Failed test 'no_symlinks' > > # at t/z_kwalitee.t line 12. > > # Error: This distribution includes symbolic links (symlinks). This > > is > > bad, because there are operating systems that do not handle symlinks. > > # Details: > > # The following symlinks were found: cmph- > > 2.0/src/.libs/libcmph.so.0;cmph-2.0/src/.libs/libcmph.so;cmph- > > 2.0/src/.libs/libcmph.la;cmph-2.0/src/.libs/libcmph.dylib;cmph- > > 2.0/lib/libcmph.so.0;cmph-2.0/lib/libcmph.so;cmph- > > 2.0/lib/libcmph.dylib > > # Remedy: Remove the symlinks from the distribution.

> > I suppose the analyser would have to check against the MANIFEST file > to be sure that the file actually was part of the distribution. > > Otherwise, you could add a skip_all if -d '.git', which would prevent > the file from running in a bare repo dir (but this would only be > useful if you were using Dist::Zilla, because otherwise the bare repo > dir is the only place tests ever run). > > Sending over to the Module-CPANTS-Analyse queue for consideration; > checking MANIFEST might be simple?

It depends on the context. As for the CPANTS website, it's not sufficient to check only manifested stuff because hand-made distributions may have symlinks that are not manifested or hidden from the CPAN indices (there're even dists without MANIFEST). OTOH, it may be reasonable to skip stuff not in the MANIFEST for Test::Kwalitee and its friends, or it may not be. (After all, should Test::Kwalitee work correctly when unnecessary stuff is still left in the dist directory even though RELEASE_TESTING is set to true?) Test::Kwalitee has already a feature to disable specific metrics (such as -no_symlinks). So I feel this is just a matter of choice by a user, not by us the maintainers. However, I can change it to behave differently depending on the context. Any ideas?

Fri Aug 08 10:34:16 2014 RURBAN [...] cpan.org - Correspondence added

On Fri Aug 08 02:50:35 2014, ISHIGAKI wrote: Show quoted text

> On Fri Aug 08 14:54:07 2014, ETHER wrote:

> > On 2014-08-07 19:15:10, RURBAN wrote:

> > > no_symlinks checks for symlinks in generated files (configure && > > > make > > > in a src subdir) > > > which are not part of the distribution. > > > > > > Perfect::Hash > > > $ pb -S prove -b t/z_kwalitee.t > > > t/z_kwalitee.t .. These tests should not be running unless > > > AUTHOR_TESTING=1 and/or RELEASE_TESTING=1! > > > t/z_kwalitee.t .. 1/? > > > # Failed test 'no_symlinks' > > > # at t/z_kwalitee.t line 12. > > > # Error: This distribution includes symbolic links (symlinks). This > > > is > > > bad, because there are operating systems that do not handle > > > symlinks. > > > # Details: > > > # The following symlinks were found: cmph- > > > 2.0/src/.libs/libcmph.so.0;cmph-2.0/src/.libs/libcmph.so;cmph- > > > 2.0/src/.libs/libcmph.la;cmph-2.0/src/.libs/libcmph.dylib;cmph- > > > 2.0/lib/libcmph.so.0;cmph-2.0/lib/libcmph.so;cmph- > > > 2.0/lib/libcmph.dylib > > > # Remedy: Remove the symlinks from the distribution.

> > > > I suppose the analyser would have to check against the MANIFEST file > > to be sure that the file actually was part of the distribution. > > > > Otherwise, you could add a skip_all if -d '.git', which would prevent > > the file from running in a bare repo dir (but this would only be > > useful if you were using Dist::Zilla, because otherwise the bare repo > > dir is the only place tests ever run). > > > > Sending over to the Module-CPANTS-Analyse queue for consideration; > > checking MANIFEST might be simple?

> > It depends on the context. As for the CPANTS website, it's not > sufficient to check only manifested stuff because hand-made > distributions may have symlinks that are not manifested or hidden from > the CPAN indices (there're even dists without MANIFEST). OTOH, it may > be reasonable to skip stuff not in the MANIFEST for Test::Kwalitee and > its friends, or it may not be. (After all, should Test::Kwalitee work > correctly when unnecessary stuff is still left in the dist directory > even though RELEASE_TESTING is set to true?) > > Test::Kwalitee has already a feature to disable specific metrics (such > as -no_symlinks). So I feel this is just a matter of choice by a user, > not by us the maintainers. However, I can change it to behave > differently depending on the context. Any ideas?

Sure. I had to use Test::Kwalitee->import(tests => ['-no_symlinks']) I found a previous report with a similar related problem: https://rt.cpan.org/Ticket/Display.html?id=26343 I believe if there is a MANIFEST it should be used. If there's none the logic can stay as it is. -- Reini Urban

Sat Aug 09 04:37:09 2014 ishigaki [...] cpan.org - Correspondence added

On Fri Aug 08 23:34:16 2014, RURBAN wrote: Show quoted text

> On Fri Aug 08 02:50:35 2014, ISHIGAKI wrote:

> > On Fri Aug 08 14:54:07 2014, ETHER wrote:

> > > On 2014-08-07 19:15:10, RURBAN wrote:

> > > > no_symlinks checks for symlinks in generated files (configure && > > > > make > > > > in a src subdir) > > > > which are not part of the distribution. > > > > > > > > Perfect::Hash > > > > $ pb -S prove -b t/z_kwalitee.t > > > > t/z_kwalitee.t .. These tests should not be running unless > > > > AUTHOR_TESTING=1 and/or RELEASE_TESTING=1! > > > > t/z_kwalitee.t .. 1/? > > > > # Failed test 'no_symlinks' > > > > # at t/z_kwalitee.t line 12. > > > > # Error: This distribution includes symbolic links (symlinks). > > > > This > > > > is > > > > bad, because there are operating systems that do not handle > > > > symlinks. > > > > # Details: > > > > # The following symlinks were found: cmph- > > > > 2.0/src/.libs/libcmph.so.0;cmph-2.0/src/.libs/libcmph.so;cmph- > > > > 2.0/src/.libs/libcmph.la;cmph-2.0/src/.libs/libcmph.dylib;cmph- > > > > 2.0/lib/libcmph.so.0;cmph-2.0/lib/libcmph.so;cmph- > > > > 2.0/lib/libcmph.dylib > > > > # Remedy: Remove the symlinks from the distribution.

> > > > > > I suppose the analyser would have to check against the MANIFEST > > > file > > > to be sure that the file actually was part of the distribution. > > > > > > Otherwise, you could add a skip_all if -d '.git', which would > > > prevent > > > the file from running in a bare repo dir (but this would only be > > > useful if you were using Dist::Zilla, because otherwise the bare > > > repo > > > dir is the only place tests ever run). > > > > > > Sending over to the Module-CPANTS-Analyse queue for consideration; > > > checking MANIFEST might be simple?

> > > > It depends on the context. As for the CPANTS website, it's not > > sufficient to check only manifested stuff because hand-made > > distributions may have symlinks that are not manifested or hidden > > from > > the CPAN indices (there're even dists without MANIFEST). OTOH, it may > > be reasonable to skip stuff not in the MANIFEST for Test::Kwalitee > > and > > its friends, or it may not be. (After all, should Test::Kwalitee work > > correctly when unnecessary stuff is still left in the dist directory > > even though RELEASE_TESTING is set to true?) > > > > Test::Kwalitee has already a feature to disable specific metrics > > (such > > as -no_symlinks). So I feel this is just a matter of choice by a > > user, > > not by us the maintainers. However, I can change it to behave > > differently depending on the context. Any ideas?

> > Sure. I had to use > Test::Kwalitee->import(tests => ['-no_symlinks']) > > I found a previous report with a similar related problem: > https://rt.cpan.org/Ticket/Display.html?id=26343 > > I believe if there is a MANIFEST it should be used. > If there's none the logic can stay as it is.

Implemented in the master. https://github.com/cpants/Module-CPANTS-Analyse/commit/05407eec87d1544a7322a2fd3ff84be58706495b

Sat Aug 09 04:37:10 2014 ishigaki [...] cpan.org - Status changed from 'open' to 'patched'

Fri Sep 26 04:41:47 2014 ishigaki [...] cpan.org - Correspondence added

On Sat Aug 09 17:37:09 2014, ISHIGAKI wrote: Show quoted text

> On Fri Aug 08 23:34:16 2014, RURBAN wrote:

> > On Fri Aug 08 02:50:35 2014, ISHIGAKI wrote:

> > > On Fri Aug 08 14:54:07 2014, ETHER wrote:

> > > > On 2014-08-07 19:15:10, RURBAN wrote:

> > > > > no_symlinks checks for symlinks in generated files (configure > > > > > && > > > > > make > > > > > in a src subdir) > > > > > which are not part of the distribution. > > > > > > > > > > Perfect::Hash > > > > > $ pb -S prove -b t/z_kwalitee.t > > > > > t/z_kwalitee.t .. These tests should not be running unless > > > > > AUTHOR_TESTING=1 and/or RELEASE_TESTING=1! > > > > > t/z_kwalitee.t .. 1/? > > > > > # Failed test 'no_symlinks' > > > > > # at t/z_kwalitee.t line 12. > > > > > # Error: This distribution includes symbolic links (symlinks). > > > > > This > > > > > is > > > > > bad, because there are operating systems that do not handle > > > > > symlinks. > > > > > # Details: > > > > > # The following symlinks were found: cmph- > > > > > 2.0/src/.libs/libcmph.so.0;cmph-2.0/src/.libs/libcmph.so;cmph- > > > > > 2.0/src/.libs/libcmph.la;cmph-2.0/src/.libs/libcmph.dylib;cmph- > > > > > 2.0/lib/libcmph.so.0;cmph-2.0/lib/libcmph.so;cmph- > > > > > 2.0/lib/libcmph.dylib > > > > > # Remedy: Remove the symlinks from the distribution.

> > > > > > > > I suppose the analyser would have to check against the MANIFEST > > > > file > > > > to be sure that the file actually was part of the distribution. > > > > > > > > Otherwise, you could add a skip_all if -d '.git', which would > > > > prevent > > > > the file from running in a bare repo dir (but this would only be > > > > useful if you were using Dist::Zilla, because otherwise the bare > > > > repo > > > > dir is the only place tests ever run). > > > > > > > > Sending over to the Module-CPANTS-Analyse queue for > > > > consideration; > > > > checking MANIFEST might be simple?

> > > > > > It depends on the context. As for the CPANTS website, it's not > > > sufficient to check only manifested stuff because hand-made > > > distributions may have symlinks that are not manifested or hidden > > > from > > > the CPAN indices (there're even dists without MANIFEST). OTOH, it > > > may > > > be reasonable to skip stuff not in the MANIFEST for Test::Kwalitee > > > and > > > its friends, or it may not be. (After all, should Test::Kwalitee > > > work > > > correctly when unnecessary stuff is still left in the dist > > > directory > > > even though RELEASE_TESTING is set to true?) > > > > > > Test::Kwalitee has already a feature to disable specific metrics > > > (such > > > as -no_symlinks). So I feel this is just a matter of choice by a > > > user, > > > not by us the maintainers. However, I can change it to behave > > > differently depending on the context. Any ideas?

> > > > Sure. I had to use > > Test::Kwalitee->import(tests => ['-no_symlinks']) > > > > I found a previous report with a similar related problem: > > https://rt.cpan.org/Ticket/Display.html?id=26343 > > > > I believe if there is a MANIFEST it should be used. > > If there's none the logic can stay as it is.

> > Implemented in the master. > > https://github.com/cpants/Module-CPANTS- > Analyse/commit/05407eec87d1544a7322a2fd3ff84be58706495b

Closed as 0.94 is released. Thanks.

Fri Sep 26 04:41:48 2014 ishigaki [...] cpan.org - Status changed from 'patched' to 'resolved'