Bug 26673 - Remove Perl shebangs from Perl modules
Summary: Remove Perl shebangs from Perl modules
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-10-14 00:24 UTC by David Cook
Modified: 2020-11-17 20:44 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06


Attachments
Bug 26673: Remove shebangs from Perl modules (3.46 KB, patch)
2020-10-14 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26673: Remove shebangs from Perl modules (3.52 KB, patch)
2020-10-14 22:54 UTC, David Cook
Details | Diff | Splinter Review
Bug 26673: Remove #!/usr/bin/env perl from .pm modules too (519 bytes, patch)
2020-10-14 22:54 UTC, David Cook
Details | Diff | Splinter Review
Bug 26673: Remove shebangs from Perl modules (3.67 KB, patch)
2020-10-15 06:12 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26673: Remove #!/usr/bin/env perl from .pm modules too (670 bytes, patch)
2020-10-15 06:12 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26673: Remove #!/usr/bin/env perl from .pm modules too (728 bytes, patch)
2020-10-15 22:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26673: Remove shebangs from Perl modules (3.72 KB, patch)
2020-10-15 22:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26673: Remove #!/usr/bin/env perl from .pm modules too (728 bytes, patch)
2020-10-15 22:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26673: Fix perlcritic complains (1.12 KB, patch)
2020-11-02 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2020-10-14 00:24:52 UTC
I've noticed a number of Perl modules have shebangs in them.

Unless there is a good reason for them to be there, they should be removed.

Examples:
C4/Barcodes/ValueBuilder.pm
C4/SIP/Logger.pm
C4/SIP/SIPServer.pm
C4/ShelfBrowser.pm
Koha/Z3950Responder.pm
Koha/Z3950Responder/GenericSession.pm
Koha/Z3950Responder/RPN.pm
Koha/Z3950Responder/Session.pm
Koha/Z3950Responder/ZebraSession.pm
Comment 1 Jonathan Druart 2020-10-14 09:25:20 UTC
Created attachment 111584 [details] [review]
Bug 26673: Remove shebangs from Perl modules

It's not needed.
Comment 2 Jonathan Druart 2020-10-14 09:41:26 UTC
 FAIL   C4/Barcodes/ValueBuilder.pm                                                                      
   FAIL   critic                                                                                         
                # Modules::RequireFilenameMatchesPackage: Got 1 violation(s).
Comment 3 Joonas Kylmälä 2020-10-14 09:44:26 UTC
(In reply to Jonathan Druart from comment #2)
>  FAIL   C4/Barcodes/ValueBuilder.pm                                         
> 
>    FAIL   critic                                                            
> 
>                 # Modules::RequireFilenameMatchesPackage: Got 1 violation(s).

Not caused by this change though, maybe you want to file separate bug for this and the same issue with Koha/Z3950Responder/RPN.pm, and actually not totally sure these are bugs.
Comment 4 Jonathan Druart 2020-10-14 09:47:42 UTC
 FAIL   Koha/Z3950Responder/RPN.pm
   FAIL   critic
                # Modules::RequireFilenameMatchesPackage: Got 1 violation(s).
Comment 5 Joonas Kylmälä 2020-10-14 09:57:41 UTC
This patch missed one instance where the shebang is not needed: C4/TTParser.pm
Comment 6 David Cook 2020-10-14 22:54:03 UTC
Created attachment 111655 [details] [review]
Bug 26673: Remove shebangs from Perl modules

It's not needed.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 7 David Cook 2020-10-14 22:54:10 UTC
Created attachment 111656 [details] [review]
Bug 26673: Remove #!/usr/bin/env perl from .pm modules too
Comment 8 David Cook 2020-10-14 22:54:53 UTC
(In reply to Joonas Kylmälä from comment #5)
> This patch missed one instance where the shebang is not needed:
> C4/TTParser.pm

Nice catch. I've added a patch for that.
Comment 9 David Cook 2020-10-14 22:58:09 UTC
(In reply to Joonas Kylmälä from comment #3)
> Not caused by this change though, maybe you want to file separate bug for
> this and the same issue with Koha/Z3950Responder/RPN.pm, and actually not
> totally sure these are bugs.

Yeah, I don't think we should fail this bug for those Perl critic failures. 

I'll open a new bug report for those though. In theory, C4/Barcodes/ValueBuilder.pm and Koha/Z3950Responder/RPN.pm would just become packages that "use" the subpackages that can be split off into their own files. (I'll take a look if there are any other packages that are guilty of the same Perl critic violation...)
Comment 10 David Cook 2020-10-14 23:09:52 UTC
(In reply to David Cook from comment #9)
> I'll open a new bug report for those though. In theory,
> C4/Barcodes/ValueBuilder.pm and Koha/Z3950Responder/RPN.pm would just become
> packages that "use" the subpackages that can be split off into their own
> files. (I'll take a look if there are any other packages that are guilty of
> the same Perl critic violation...)

Ok C4/Barcodes/ValueBuilder.pm was easy, but I just realized Koha/Z3950Responder/RPN.pm overrides a non-C4/non-Koha namespace. Blah. This is where our terrible codetree layout comes to bite us heh.
Comment 11 David Cook 2020-10-15 00:56:11 UTC
Ok I've opened Bug 26683 to deal with the Perl critic issues
Comment 12 Joonas Kylmälä 2020-10-15 06:12:30 UTC
Created attachment 111677 [details] [review]
Bug 26673: Remove shebangs from Perl modules

It's not needed.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 13 Joonas Kylmälä 2020-10-15 06:12:36 UTC
Created attachment 111678 [details] [review]
Bug 26673: Remove #!/usr/bin/env perl from .pm modules too

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 14 Katrin Fischer 2020-10-15 22:32:16 UTC
Created attachment 111810 [details] [review]
Bug 26673: Remove #!/usr/bin/env perl from .pm modules too

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2020-10-15 22:32:52 UTC
Created attachment 111811 [details] [review]
Bug 26673: Remove shebangs from Perl modules

It's not needed.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Katrin Fischer 2020-10-15 22:32:59 UTC
Created attachment 111812 [details] [review]
Bug 26673: Remove #!/usr/bin/env perl from .pm modules too

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2020-10-15 22:33:12 UTC
Could we have a QA script check for this?
Comment 18 David Cook 2020-10-16 00:13:58 UTC
(In reply to Katrin Fischer from comment #17)
> Could we have a QA script check for this?

Oooh that's a good idea!
Comment 19 Jonathan Druart 2020-10-25 23:08:11 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 20 Jonathan Druart 2020-11-02 07:42:17 UTC
David, this change makes t/00-testcritic.t fail on

#   Failed test 'Test::Perl::Critic for "C4/Barcodes/ValueBuilder.pm"'
#   at /usr/share/perl5/Test/Perl/Critic.pm line 121.
# 
#   Package declaration must match filename at line 19, column 1.  Correct the filename or package statement.  (Severity: 5)

#   Failed test 'Test::Perl::Critic for "Koha/Z3950Responder/RPN.pm"'
#   at /usr/share/perl5/Test/Perl/Critic.pm line 121.
# 
#   Package declaration must match filename at line 1, column 1.  Correct the filename or package statement.  (Severity: 5)

I am aware of bug 26683 but I'd like a quick fix to have the tests green now.
Comment 21 Jonathan Druart 2020-11-02 07:43:15 UTC
Created attachment 112814 [details] [review]
Bug 26673: Fix perlcritic complains

This patch fixes the following perlcritic complain:
Package declaration must match filename at line 19, column 1.  Correct the filename or package statement.
Comment 22 Jonathan Druart 2020-11-02 10:06:18 UTC
(In reply to Jonathan Druart from comment #21)
> Created attachment 112814 [details] [review] [review]
> Bug 26673: Fix perlcritic complains
> 
> This patch fixes the following perlcritic complain:
> Package declaration must match filename at line 19, column 1.  Correct the
> filename or package statement.

Pushed to master for 20.11
Comment 23 Lucas Gass 2020-11-14 02:32:24 UTC
backported to 20.05.x for 20.05.06
Comment 24 Aleisha Amohia 2020-11-17 04:44:03 UTC
enhancement, not backported to 19.11.x