Skip to content
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

Implemented copyright checker for new and modified files #3890

Merged
merged 1 commit into from
May 19, 2022

Conversation

catlauraherzog
Copy link

@catlauraherzog catlauraherzog commented Jan 11, 2022

I implemented a check if the copyright lines are at a correct place and contain the exact wording. For every modified and new file the lines are defined as:

/**
 * This file is part of ILIAS, a powerful learning management system
 * published by ILIAS open source e-Learning e.V.
 *
 * ILIAS is licensed with the GPL-3.0,
 * see https://www.gnu.org/licenses/gpl-3.0.en.html
 * You should have received a copy of said license along with the
 * source code, too.
 *
 * If this is not the case or you just want to try ILIAS, you'll find
 * us at:
 * https://www.ilias.de
 * https://github.com/ILIAS-eLearning
 */

They need to start at line 2 or 3.

@chfsx
Copy link
Member

chfsx commented Jan 21, 2022

Hi @lauraherzog
I tried the checker in the following setting:

  • used your branch lauraherzog:copyright
  • adopted the script a bit to just scan Service/ActiveRecord (which has no license header in the state of your branch)
  • added the license header to Service/ActiveRecord-Files with ./libs/composer/vendor/bin/rector process --clear-cache --xdebug --no-diffs --config ./CI/Rector/rector.php Services/ActiveRecord
  • run the (modified) checker

as far I can see all files of Services/ActiveRecord get listed as Copyright not as expected in .... Maybe I'm doint something wrong :-)

Copy link
Member

@chfsx chfsx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

@catlauraherzog
Copy link
Author

@chfsx I don't think you are doing anything wrong. I just think that my script is not bulletproof so we need to find the error :)

I see that you modified Services/ActiveRecord with this Rectorthing (which I don't know). Could you give me an example of two or three modified files (just the header with the copyright statement)?

Best, Laura

@chfsx
Copy link
Member

chfsx commented Feb 11, 2022

Hi @lauraherzog
I still fail to run the check for files, wich have a license-header introduces by rector. E.G. Services/ActiveRecord has automatically added License-header (currently in trunk without whitespaces before the two https://... like in your check). could not find a difference between the header you check against and the headers in the files... I assume the check is too hard and should ignore some things like whitespaces.

i changes the line 20 in copyright-checker.sh to just check Services/ActiveRecord like this:
CHANGED_FILES=$(find Services/ActiveRecord -path ./libs -prune -o -type f -name '*.php')

@catlauraherzog
Copy link
Author

Hey @chfsx

I reworked the checkmodel by removing whitespace, stars and slashes. My tests worked with the copyright notice from above. Additionally I tested with notices where I added stars and slashes. So the test is a bit weaker.

Does that satisfy our needs?

@chfsx
Copy link
Member

chfsx commented Feb 11, 2022

we are almost there :-) I made a branch with your checker and changes a lot of files with the license header. the action runs und succeeds, but have a look at the step "Copyright Check" here
https://github.com/srsolutionsag/ILIAS/runs/5157170888?check_suite_focus=true

it doesn't seem to run:


Run CI/Copyright-Checker/copyright-checker.sh
jq: error (at <stdin>:4): Cannot index string with string "filename"
Scanning changed files for copyright notice ...
Scan complete. Found 0 incidents.

no Idea where this comes from, maybe just because it's not yet part of the repo?

@klees
Copy link
Member

klees commented May 12, 2022

@lauraherzog @chfsx What's the status here?

@chfsx
Copy link
Member

chfsx commented May 12, 2022

@klees from my point of view the same as in 11 Feb, but did not check it again.

@catlauraherzog
Copy link
Author

@lauraherzog @chfsx What's the status here?

@klees @chfsx Fixed the implementation. Could you verify pls?

Copy link
Member

@chfsx chfsx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lauraherzog, thanks a lot for the implementation, seems to work as expected now!

@chfsx chfsx merged commit 41d1aff into ILIAS-eLearning:trunk May 19, 2022
@chfsx
Copy link
Member

chfsx commented May 19, 2022

Thank you very much @lauraherzog !

@chfsx
Copy link
Member

chfsx commented May 19, 2022

Ok the new check is quite hard :) I have my first failed test now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants