Bug 29324 - Some files still don't have the correct license statement
Summary: Some files still don't have the correct license statement
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Philip Orr
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 24545
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-26 14:09 UTC by Jonathan Druart
Modified: 2023-10-02 09:17 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 29324: Adjust tests (1.52 KB, patch)
2021-10-26 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29324: Adjust tests (1.52 KB, patch)
2023-07-11 10:26 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 29324: Change license statements in various files (24.12 KB, patch)
2023-07-11 10:26 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 29324: Adjust tests (1.52 KB, patch)
2023-07-17 13:58 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 29324: Changed patch to correct copyright (15.82 KB, patch)
2023-07-17 13:58 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 29324: Adjust tests (1.57 KB, patch)
2023-07-19 17:11 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29324: Changed patch to correct copyright (15.87 KB, patch)
2023-07-19 17:11 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29324: change test to look for copyright (1.55 KB, patch)
2023-07-25 07:43 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 29324: Change test to look at all .pm or .pl files (2.03 KB, patch)
2023-07-26 08:23 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 29324: (QA follow-up) Polishing (3.09 KB, patch)
2023-07-26 13:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29324: (QA follow-up) Polishing (3.60 KB, patch)
2023-08-04 09:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29324: Adjust tests (1.57 KB, patch)
2023-09-27 09:09 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 29324: Changed patch to correct copyright (15.87 KB, patch)
2023-09-27 09:09 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 29324: change test to look for copyright (1.55 KB, patch)
2023-09-27 09:09 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 29324: Change test to look at all .pm or .pl files (2.03 KB, patch)
2023-09-27 09:09 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 29324: (QA follow-up) Polishing (3.56 KB, patch)
2023-09-27 09:09 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 29324: Prepend copyright or license statement (4.81 KB, patch)
2023-09-27 09:09 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 29324: Add missing copyright and license info (876 bytes, patch)
2023-09-27 09:09 UTC, Philip Orr
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-10-26 14:09:29 UTC

    
Comment 1 Jonathan Druart 2021-10-26 14:10:00 UTC
Created attachment 126916 [details] [review]
Bug 29324: Adjust tests
Comment 2 Jonathan Druart 2021-10-26 14:10:41 UTC
Anybody willing to make those tests pass?

The copyright/license statement from Koha/Biblio.pm needs to be copied to the different files that make this test fails.
Comment 3 Philip Orr 2023-07-07 14:33:48 UTC
Hmm, I didn't do anything but apply the patch and run the test and the tests already aren't failing for me.

kohadev-koha@kohadevbox:xt(bug29324)$ perl find-license-problems.t
ok 1
ok 2
ok 3
ok 4
ok 5
ok 6
ok 7
ok 8
ok 9
ok 10
ok 11
ok 12
ok 13
ok 14
ok 15
1..15
Comment 4 Jonathan Druart 2023-07-11 08:31:42 UTC
(In reply to Philip Orr from comment #3)
> Hmm, I didn't do anything but apply the patch and run the test and the tests
> already aren't failing for me.
> 
> kohadev-koha@kohadevbox:xt(bug29324)$ perl find-license-problems.t
> ok 1
> ok 2
> ok 3
> ok 4
> ok 5
> ok 6
> ok 7
> ok 8
> ok 9
> ok 10
> ok 11
> ok 12
> ok 13
> ok 14
> ok 15
> 1..15

Hi Philip,
This is still valid, there are more than 15 tests. You need to run the script from the root of the Koha src directory, not from xt.

You will get:

#   Failed test at xt/find-license-problems.t line 63.                                                                                                                                                               
#          got: '0'
#     expected: '1'                                                                                       
# File ./Koha/Course.pm has wrong copyright: hasgpl=1, hasv3=1, hasorlater=1, haslinktolicense=0, hasfranklinst=1
                                                     
#   Failed test at xt/find-license-problems.t line 63.                                                                                                                                                               
#          got: '0'
#     expected: '1'                                                                                       
# File ./Koha/Courses.pm has wrong copyright: hasgpl=1, hasv3=1, hasorlater=1, haslinktolicense=0, hasfranklinst=1

And so on...
Comment 5 Philip Orr 2023-07-11 08:40:58 UTC
Hi Jonathan,
ah, makes sense. I get the failing tests now. Thanks!
Comment 6 Philip Orr 2023-07-11 10:26:29 UTC
Created attachment 153309 [details] [review]
Bug 29324: Adjust tests
Comment 7 Philip Orr 2023-07-11 10:26:31 UTC Comment hidden (obsolete)
Comment 8 Philip Orr 2023-07-11 10:32:19 UTC
Just a note on this patch:
Two files already contained parts copyright or author statements.

misc/translator/tmpl_process3.pl contained the following:
# Parts copyright 2003-2004 Paul Poulain
# Parts copyright 2003-2004 Jerome Vizcaino
# Parts copyright 2004 Ambrose Li

reports/orders_by_fund.pl contained:
# Author : Frédérick Capovilla, 2011 - SYS-TECH
# Modified by : Élyse Morin, 2012 - Libéo

I left those statements in the files and only added to them the
other missing license or copyright info.
Comment 9 Katrin Fischer 2023-07-16 11:41:30 UTC
Hi Philip,

happy to see you submitting patches!

It looks like all files now have these lines included:

+# Copyright 2000-2002 Katipo Communications
+# Copyright 2010 BibLibre
+# Copyright 2011 Equinox Software, Inc.

This doesn't seem right to me. Should we not just update the license statements (This file is part of Koha...) here?

https://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
Comment 10 Philip Orr 2023-07-17 07:47:33 UTC
Hi Katrin,

happy to be able to :-)
Hmm, I see what you mean.

However find-license-problems.t fails if there isn't at least one copyright statement in the file, and some of them didn't have a statement yet.
For example see the diff in my patch for Koha/Course.pm.
So how do we handle those cases?

Jonathan mentioned:
>>The copyright/license statement from Koha/Biblio.pm needs to be copied to the >>different files that make this test fails.

Is that because Koha/Biblio.pm has the same authors as e.g. Course.pm?
Comment 11 Katrin Fischer 2023-07-17 07:51:47 UTC
Hm, I am not sure how the test works, but giving those 3 copyright to files they might have never touched seems not right :) Maybe Joubu can help at least on how the test is supposed to be read?
Comment 12 Jonathan Druart 2023-07-17 08:17:06 UTC
Files are supposed to have a copyright statement. We use to add "# Copyright YEAR Koha Development Team", even if it's not 100% correct (Koha Development TEAM is not a legal entity) at least it seems better than no statement.
Comment 13 Katrin Fischer 2023-07-17 09:50:44 UTC
(In reply to Jonathan Druart from comment #12)
> Files are supposed to have a copyright statement. We use to add "# Copyright
> YEAR Koha Development Team", even if it's not 100% correct (Koha Development
> TEAM is not a legal entity) at least it seems better than no statement.

Sounds good to me. YEAR being the current year?
Comment 14 Jonathan Druart 2023-07-17 12:49:26 UTC
(In reply to Katrin Fischer from comment #13)
> YEAR being the current year?

Yes
Comment 15 Philip Orr 2023-07-17 12:59:58 UTC
OK, I'll add a new patch!
Comment 16 Philip Orr 2023-07-17 13:58:20 UTC
Created attachment 153555 [details] [review]
Bug 29324: Adjust tests
Comment 17 Philip Orr 2023-07-17 13:58:23 UTC
Created attachment 153556 [details] [review]
Bug 29324: Changed patch to correct copyright

This changes the previous patch to add the correct copyright and license statements to various files
that didn't contain them.
To test:
1. Before applying the patch, run find-license-problems.t from the
   root of your Koha src directory:
   $ perl xt/find-license-problems.t
2. Observe that 14 tests fail.
3. Apply the patch.
4. Run the test again
5. Observe that no tests fail.
Comment 18 Owen Leonard 2023-07-19 17:11:29 UTC
Created attachment 153680 [details] [review]
Bug 29324: Adjust tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 19 Owen Leonard 2023-07-19 17:11:31 UTC
Created attachment 153681 [details] [review]
Bug 29324: Changed patch to correct copyright

This changes the previous patch to add the correct copyright and
license statements to various files that didn't contain them.
To test:
1. Before applying the patch, run find-license-problems.t from the
   root of your Koha src directory:
   $ perl xt/find-license-problems.t
2. Observe that 14 tests fail.
3. Apply the patch.
4. Run the test again
5. Observe that no tests fail.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 20 Marcel de Rooy 2023-07-21 09:56:56 UTC
Just noting that I tend to write Koha development team. Inline with other not all caps stuff.

I opened a few Koha modules and saw a lot of them missing Copyright lines.

If we are fixing this script, lets add a test now too for the Copyright line.
Comment 21 Philip Orr 2023-07-21 13:25:21 UTC
Hi Marcel,

thanks for QA'ing! The test should already check if a file is missing a Copyright line, something must be going wrong there. I'll take a look at it and see if I can figure it out.
Just to be sure - can you give me an example file where Copyright line is missing?
Comment 22 Marcel de Rooy 2023-07-24 06:00:44 UTC
(In reply to Philip Orr from comment #21)
> Just to be sure - can you give me an example file where Copyright line is
> missing?

There are lots.

Just in Koha folder:
grep -L "Copyright" *.pm| wc -l
83
Comment 23 Philip Orr 2023-07-25 07:43:20 UTC
Created attachment 153861 [details] [review]
Bug 29324: change test to look for copyright

This patch makes some changes to the test file find-license-problems.t
so that it will also check if the file contains a copyright statement
and fail if it doesn't. Note that the test file now only checks files
that end with .pm, other file types are ignored.

To test:
1. Apply the patch
2. run find-license-problems.t from the
   root of your Koha src directory:
   $ perl xt/find-license-problems.t
3. Note that now many more files are failing the test than before.
Comment 24 Philip Orr 2023-07-25 07:45:38 UTC
This might be a stupid question, so sorry for that..
Should we only be checking .pm files for copyright / license problems?
Shouldn't we also be checking .pl, .js ... ?
Comment 25 Marcel de Rooy 2023-07-25 09:29:10 UTC
(In reply to Philip Orr from comment #24)
> This might be a stupid question, so sorry for that..
> Should we only be checking .pm files for copyright / license problems?
> Shouldn't we also be checking .pl, .js ... ?

Sure. I just gave you few examples.
In the .pm and .pl we normally add the license and copyright.
A lot of js is in the .tt files without license/copyright.
There are indeed some separate js files. They normally dont contain that info. And I agree that they probably should. But perhaps move that to another report..
Comment 26 Philip Orr 2023-07-25 09:33:42 UTC
OK, got it. Thanks Marcel for the feedback!
I'll change the test and then get to work on the files themselves, staying on failed QA until then.
Comment 27 Philip Orr 2023-07-26 08:23:27 UTC
Created attachment 153899 [details] [review]
Bug 29324: Change test to look at all .pm or .pl files

This patch changes the test at xt/find-license-problems.t to look
at all .pm or .pl files. Other files will only be checked if
they contain the message "This file is part of Koha".

To test:
1. Apply the patch
2. Run find-license-problems.t from the
   root of your Koha src directory:
   $ perl xt/find-license-problems.t
3. You should be getting 1476 fails
Comment 28 Marcel de Rooy 2023-07-26 12:34:40 UTC
(In reply to Philip Orr from comment #27)
> 3. You should be getting 1476 fails
Wow

Maybe we should ignore this directory:

File ./installer/data/mysql/db_revs/211200038.pl has wrong copyright

We wont be adding license and copyright to all those small dbrevs.
Comment 29 Philip Orr 2023-07-26 12:58:42 UTC
Sounds good. Though that's "only" about 270 files.
Around half of the files are missing both copyright and license statements (ca. 750). The other half are only missing copyright statements.

Different question though. Some files (for instance Koha/AdditionalFields.pm) include the copyright and license statement as part of pod documentation.
In that case, should I just leave the file as-is or should I add license/copyright statements commented out at the beginning of the file like in Koha/Biblio.pm?
Or something else? :)
Comment 30 Marcel de Rooy 2023-07-26 13:05:44 UTC
Created attachment 153924 [details] [review]
Bug 29324: (QA follow-up) Polishing

Making the verbose output a bit more manageable with -v option.
If we just run the test, we get the total of files with license
problems.
Comment 31 Marcel de Rooy 2023-07-26 13:07:08 UTC
(In reply to Philip Orr from comment #29)

> Different question though. Some files (for instance
> Koha/AdditionalFields.pm) include the copyright and license statement as
> part of pod documentation.
> In that case, should I just leave the file as-is or should I add
> license/copyright statements commented out at the beginning of the file like
> in Koha/Biblio.pm?

If it is in POD, it should be fine too. No need to change then..
Comment 32 Marcel de Rooy 2023-07-26 13:07:47 UTC
Please see follow-up too. Run script with -v to get the sprintf output
Comment 33 Marcel de Rooy 2023-07-26 13:08:17 UTC
1..1
not ok 1 - No files without license problems anymore!
#   Failed test 'No files without license problems anymore!'
#   at xt/find-license-problems.t line 68.
#          got: '1218'
#     expected: '0'
# Looks like you failed 1 test of 1.
Comment 34 Marcel de Rooy 2023-07-26 13:25:55 UTC
-    next unless $hascopyright || $needs_copyright;
+    next unless $needs_copyright &&!$hascopyright;

This might need attention still.
Comment 35 Philip Orr 2023-07-28 06:46:56 UTC
(In reply to Marcel de Rooy from comment #34)
> -    next unless $hascopyright || $needs_copyright;
> +    next unless $needs_copyright &&!$hascopyright;
> 
> This might need attention still.

Hm, the way I understood the original test was that files that already have copyright statements should still be checked (because we also want to check for license statement problems e.g. Franklin Street or gpl v2... etc., even if there's already a copyright statement in the file).

Btw thanks for the verbose flag, that makes it much easier :)
Comment 36 Marcel de Rooy 2023-08-04 09:25:46 UTC
Created attachment 154240 [details] [review]
Bug 29324: (QA follow-up) Polishing

Making the verbose output a bit more manageable with -v option.
If we just run the test, we get the total of files with license
problems.

Added Koha/Schema/Result as exception.

Sorts results on file name.
Comment 37 Marcel de Rooy 2023-08-04 09:26:23 UTC
(In reply to Philip Orr from comment #35)
> Hm, the way I understood the original test was that files that already have
> copyright statements should still be checked (because we also want to check
> for license statement problems e.g. Franklin Street or gpl v2... etc., even
> if there's already a copyright statement in the file).

I fixed it. Restored the old condition.
Comment 38 Philip Orr 2023-09-27 09:09:14 UTC
Created attachment 156256 [details] [review]
Bug 29324: Adjust tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 39 Philip Orr 2023-09-27 09:09:17 UTC
Created attachment 156257 [details] [review]
Bug 29324: Changed patch to correct copyright

This changes the previous patch to add the correct copyright and
license statements to various files that didn't contain them.
To test:
1. Before applying the patch, run find-license-problems.t from the
   root of your Koha src directory:
   $ perl xt/find-license-problems.t
2. Observe that 14 tests fail.
3. Apply the patch.
4. Run the test again
5. Observe that no tests fail.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 40 Philip Orr 2023-09-27 09:09:20 UTC
Created attachment 156258 [details] [review]
Bug 29324: change test to look for copyright

This patch makes some changes to the test file find-license-problems.t
so that it will also check if the file contains a copyright statement
and fail if it doesn't. Note that the test file now only checks files
that end with .pm, other file types are ignored.

To test:
1. Apply the patch
2. run find-license-problems.t from the
   root of your Koha src directory:
   $ perl xt/find-license-problems.t
3. Note that now many more files are failing the test than before.
Comment 41 Philip Orr 2023-09-27 09:09:22 UTC
Created attachment 156259 [details] [review]
Bug 29324: Change test to look at all .pm or .pl files

This patch changes the test at xt/find-license-problems.t to look
at all .pm or .pl files. Other files will only be checked if
they contain the message "This file is part of Koha".

To test:
1. Apply the patch
2. Run find-license-problems.t from the
   root of your Koha src directory:
   $ perl xt/find-license-problems.t
3. You should be getting 1476 fails
Comment 42 Philip Orr 2023-09-27 09:09:25 UTC
Created attachment 156260 [details] [review]
Bug 29324: (QA follow-up) Polishing

Making the verbose output a bit more manageable with -v option.
If we just run the test, we get the total of files with license
problems.

Added Koha/Schema/Result as exception.

Sorts results on file name.
Comment 43 Philip Orr 2023-09-27 09:09:28 UTC Comment hidden (obsolete)
Comment 44 Philip Orr 2023-09-27 09:09:30 UTC
Created attachment 156262 [details] [review]
Bug 29324: Add missing copyright and license info

This patch adds a new perl file to prepend
copyright and/or license statements to all files
that are missing them in the directory
(and subdirectories) in which it is run.

To test:
1. Apply the patch
2. Run perl xt/find-license-problems.t
3. Note there are some fails
4. Run perl xt/prependcopyrightandlicense.pl
5. Run perl xt/find-license-problems.t again
6. No more fails :)
Comment 45 Philip Orr 2023-09-27 10:02:50 UTC
Oops.. just noticed, with this patch, I would be prepending the copyright/license info before the shebang. Setting back to failed QA.
Comment 46 Philip Orr 2023-10-02 09:17:13 UTC
Thought about this one a little more over the weekend.
Perl files will need the shebang to be the first line.
For other files, (I hope) it doesn't matter if copyright/license
statements are the first line.
What I'm going to try is something like:
- if the first line is a shebang, add copyright/license info starting
from the second line
- if not, add copyright/license info directly from the first line