Skip to content

Add support of scancode_cli to CoLic Backend #29

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 28, 2019
Merged

Add support of scancode_cli to CoLic Backend #29

merged 3 commits into from
May 28, 2019

Conversation

inishchith
Copy link
Contributor

Addresses: #27

@valeriocos In reference to your PR. This contains some minor changes.
Still, there are some improvements which can be made and tests should be added accordingly.
Please do have a look, Thanks :)

Work attributions: @valeriocos 's #28

@inishchith inishchith force-pushed the add_scancli branch 3 times, most recently from cc70d2e to 62dd886 Compare May 15, 2019 18:50
valeriocos
valeriocos reviewed May 16, 2019
View reviewed changes
valeriocos
valeriocos reviewed May 16, 2019
View reviewed changes
valeriocos
valeriocos reviewed May 16, 2019
View reviewed changes
@valeriocos
Copy link
Member

Thank you for addressing the comments @inishchith

@inishchith
Copy link
Contributor Author

@valeriocos I've updated the PR.
I've made the changes wrt. the assumption that exec_path provided would always point to the scancode executable. Please let me know if we're keeping it as the default convention of usage.

Thanks

@valeriocos
Copy link
Member

Sorry for the late reply @inishchith .
To avoid confusion It is probably better if exec_path points to the file that should be executed (either scancode or scancli.py). Why exec_pathshould always point toscancode` ?

@inishchith
Copy link
Contributor Author

@valeriocos This came to my mind as I referred your implementation here and thought if we were planning to do so, now i understand that it was for a quick test.
Thanks for the clarification. I'll make the changes

@valeriocos
Copy link
Member

perfect, thanks! That branch was a quick test, PR #28 should be better :)

@inishchith
Copy link
Contributor Author

@valeriocos I've made the changes. Please do have a look
I moving ahead to working on tests :)

@valeriocos
Copy link
Member

Great @inishchith , thank you for the updates. Please ping when the tests are ready! :)

@inishchith
Copy link
Contributor Author

inishchith commented May 21, 2019 •
edited
Loading

@valeriocos I had a thought before starting work on tests.

We are required to accommodate configuring scancli.py before the tests start. So, I was thinking to perform

git checkout -b xxx 8afa686fb71b9540029234e5a40c0572c4457c28

above checkout commit and 1st and 2nd step from this doc in build under before_script under travisci configuration.

Edit: I think we have to use a clone instead of a release as the latest release of scancode-toolkit was on 15th Feb, and scancli was added later ( 5th march ).
I didn't realize that I tested my work on a clone, instead of v3.0.0.

Please let me know what you think :)

@inishchith
Copy link
Contributor Author

inishchith commented May 22, 2019 •
edited
Loading

@valeriocos I'm not sure why there's no travis-ci and coverage triggered on last push.
Can you please check?

Edit: It took some more time to start 🙈

@inishchith inishchith force-pushed the add_scancli branch 4 times, most recently from 4edb2a4 to ee1f6a4 Compare May 23, 2019 05:02
@inishchith
Copy link
Contributor Author

inishchith commented May 23, 2019 •
edited
Loading

@valeriocos Please review when you get time
Thanks :)

Note:

  • The earlier checkout commit caused error for running scancode. hence had to find a later commit.

Edit:

  • Was trying to improve coverage 🎉

@inishchith inishchith force-pushed the add_scancli branch 2 times, most recently from 517c0fa to df1d195 Compare May 25, 2019 05:56
@valeriocos
Copy link
Member

I'm on it @inishchith :)

valeriocos
valeriocos reviewed May 26, 2019
View reviewed changes
Copy link
Member

@valeriocos valeriocos left a comment

Choose a reason for hiding this comment

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

Sorry for the late review @inishchith . I left some comments, but overall the PR looks good, though some extra work is needed:

  • Probably the readme should include some details to use scancode and scancli (for instance the steps defined in the travis.yml could be really useful). If you agree, the new changes could go to a new commit.
  • The first and third commit could be merged together and the description improved. For instance, the new description could include an example of how to run scancode cli. The description of the second commit (the one about travis) could also be improved, for instance some details could be included to explain why we use the clone instead of a release.

What do you think ?

@inishchith
Copy link
Contributor Author

@valeriocos
I've gone through the comments and I see there are improvements needed.
I'll work on them and update you accordingly :)

Thanks

@valeriocos
Copy link
Member

Great! Thank you @inishchith to check the comments

@inishchith
Copy link
Contributor Author

@valeriocos I've worked on the requested changes.

  • Merged (earlier) commits 1 & 3 into 1, made the requested changes and updated commit description.
  • Updated the description of 2nd commit (travis.yml)
  • Added installation and usage guide to README

Please let me know if I've missed something

Thanks

@inishchith
Copy link
Contributor Author

@valeriocos Please do have a look when you get time :)
Thanks

valeriocos
valeriocos reviewed May 28, 2019
View reviewed changes
valeriocos
valeriocos reviewed May 28, 2019
View reviewed changes
Copy link
Member

@valeriocos valeriocos left a comment

Choose a reason for hiding this comment

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

Thank you @inishchith , it looks great, there is just a minor comment to address, and then we can merge it.

inishchith added 3 commits May 28, 2019 21:54
@inishchith
[colic] Add support of scancode_cli and it's corresponding tests to c…
ec7f195 
…olic backend

A faster version of scancode is now added to colic backend
scancode_cli usage:

colic https://github.com/chaoss/grimoirelab-toolkit
--git-path /tmp/scancode_cli
--exec-path /home/scancode-toolkit/etc/scripts/scancli.py
--category code_license_scancode_cli

Added tests for scancode_cli analyzer which also required updating tests
for in-place implementation of colic backend.

Signed-off-by: inishchith <[email protected]>
@inishchith
[travis] Update scancode-toolkit configuration
d63a8e5 
We're now using a clone of scancode-toolkit instead of a release, as the
latest release is of 15th February 2019 and the scancli.py script was
incroporated later i.e 5th March 2019 and there hasn't been a release
since.

Add stable scancode_cli checkout commit in order to accomodate functioning
of both scancode & scancode_cli

Signed-off-by: inishchith <[email protected]>
@inishchith
[README] Add installation and usage details of scancode_cli analyzer
bb9e355 
Signed-off-by: inishchith <[email protected]>
@inishchith
Copy link
Contributor Author

@valeriocos Thanks for the review
I've updated the doc-string, please do have a look :)

valeriocos
valeriocos approved these changes May 28, 2019
View reviewed changes
Copy link
Member

@valeriocos valeriocos left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @inishchith

@valeriocos valeriocos merged commit bb9e355 into chaoss:master May 28, 2019
valeriocos added a commit that referenced this pull request May 28, 2019
@valeriocos
Merge branch 'add_scancli' of 'https://github.com/inishchith/graal
100ba96 
Merges #29
@valeriocos valeriocos changed the title Add support of scancode_cli to CoLic Backend ( WIP ) Add support of scancode_cli to CoLic Backend May 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@valeriocos valeriocos valeriocos approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@inishchith @valeriocos