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

Report multiple LICENSE files #78

Closed
wants to merge 10 commits into from
Closed

Report multiple LICENSE files #78

wants to merge 10 commits into from

Conversation

johnthagen
Copy link
Contributor

@johnthagen johnthagen commented Aug 30, 2020

Closes #71

Relates to 3.0.0 release #77

Open question: Should we change --with-license-file to --with-license-files for added clarity?

Unit tests will be updated once consensus is reached on the approach/design.

Tested against the opencv-python-headless package, which has two included LICENSE files.

Supported:

  • PlainVertical
  • JSON
  • JSON LicenseFinder (does not support License files anyway)
  • Plain (see comment)
  • HTML
  • CSV
  • RST
  • Markdown
  • Confluence

@johnthagen
Copy link
Contributor Author

By default plain looks something like (with truncated license files):

 Name                    Version   License  LicenseFile                                                                                                                                                                                                                                                       LicenseText                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                          
 opencv-python-headless  4.4.0.42  MIT      ['c:\\users\\user\\github\\pip-licenses\\venv\\lib\\site-packages\\opencv_python_headless-4.4.0.42.dist-info\\LICENSE-3RD-PARTY.txt', 'c:\\users\\user\\github\\pip-licenses\\venv\\lib\\site-packages\\opencv_python_headless-4.4.0.42.dist-info\\LICENSE.txt']  ['OpenCV library is redistributed within opencv-python package.\nThis license applies to OpenCV binary in the directory cv2/.\n\nBy downloading, copying, installing or using the software you agree to this license.\nIf you do not agree to this license, do not download, install,\ncopy or use the software.\n\n\n                          License Agreement\n               For Open Source Computer Vision Library\n                       (3-clause BSD License)\n\n', 'MIT License\n\nCopyright (c) Olli-Pekka Heinisuo\n\nPermission is hereby granted, free of charge, to any person obtaining a copy\n'] 

Personally, this doesn't seem much worse than the status quo formatting wise, and also conveys all of the information. Not sure if people would actually use plain format with license files or not, but it is deliminated as a Python list.

@johnthagen
Copy link
Contributor Author

HTML:

<table>
    <tr>
        <th>Name</th>
        <th>Version</th>
        <th>License</th>
        <th>LicenseFile</th>
        <th>LicenseText</th>
    </tr>
    <tr>
        <td>opencv-python-headless</td>
        <td>4.4.0.42</td>
        <td>MIT</td>
        <td>[&#x27;c:\\users\\user\\github\\pip-licenses\\venv\\lib\\site-packages\\opencv_python_headless-4.4.0.42.dist-info\\LICENSE-3RD-PARTY.txt&#x27;, &#x27;c:\\users\\user\\github\\pip-licenses\\venv\\lib\\site-packages\\opencv_python_headless-4.4.0.42.dist-info\\LICENSE.txt&#x27;]</td>
        <td>[&#x27;OpenCV library is redistributed within opencv-python package.\nThis license applies to OpenCV binary in the directory cv2/.\n\nBy downloading, copying, installing or using the software you agree to this license.\nIf you do not agree to this license, do not download, install,\ncopy or use the software.\n\n\n                          License Agreement\n               For Open Source Computer Vision Library\n                       (3-clause BSD License)\n\n&#x27;, &#x27;MIT License\n\nCopyright (c) Olli-Pekka Heinisuo\n\nPermission is hereby granted, free of charge, to any person obtaining a copy\n&#x27;]</td>
    </tr>
</table>

Ideally these could be broken into a list using <ul>, but this is implemented in the prettytable.get_html_string() so it is not easily modifiable in this project.

@johnthagen
Copy link
Contributor Author

CSV:

"Name","Version","License","LicenseFile","LicenseText"
"opencv-python-headless","4.4.0.42","MIT","['c:\\users\\user\\github\\pip-licenses\\venv\\lib\\site-packages\\opencv_python_headless-4.4.0.42.dist-info\\LICENSE-3RD-PARTY.txt', 'c:\\users\\user\\github\\pip-licenses\\venv\\lib\\site-packages\\opencv_python_headless-4.4.0.42.dist-info\\LICENSE.txt']","['OpenCV library is redistributed within opencv-python package.\nThis license applies to OpenCV binary in the directory cv2/.\n\nBy downloading, copying, installing or using the software you agree to this license.\nIf you do not agree to this license, do not download, install,\ncopy or use the software.\n\n\n                          License Agreement\n               For Open Source Computer Vision Library\n                       (3-clause BSD License)\n\n', 'MIT License\n\nCopyright (c) Olli-Pekka Heinisuo\n\nPermission is hereby granted, free of charge, to any person obtaining a copy\n']"

We could customize this in CSVPrettyTable but I'm not sure what the best way forward is here, or if we should even try to support CSV for --with-license-files.

@johnthagen
Copy link
Contributor Author

@raimon49 Let me know if you have feedback/ideas on the direction of this. I really like how plain-vertical and json look (the two most-likely used options with license files). I'm unsure exactly the best choice for the others (or if they should even be supported with --with-license-files).

@raimon49
Copy link
Owner

raimon49 commented Aug 31, 2020

@johnthagen Thanks for the PR.

First, I would like the base branch of PR to be release-3.0 instead of master. And it's better if the branch branch source is rebased to release-3.0.
image

Personally, this doesn't seem much worse than the status quo formatting wise, and also conveys all of the information. Not sure if people would actually use plain format with license files or not, but it is deliminated as a Python list.

Yes, I agree with you. This is a surprisingly good look.

Ideally these could be broken into a list using <ul>, but this is implemented in the prettytable.get_html_string() so it is not easily modifiable in this project.

Yes, this is indeed a bad look.

Even though it's hard to replace <ul> - <li> with <td>, it's better to be served like a Python list in a single <td> element.

We could customize this in CSVPrettyTable but I'm not sure what the best way forward is here, or if we should even try to support CSV for --with-license-files.

I think that if there is a single license file found, output it without enclosing it in [].

I really like how plain-vertical and json look (the two most-likely used options with license files). I'm unsure exactly the best choice for the others (or if they should even be supported with --with-license-files).

If you replace a valid option with --with-license-files, you will be warned when a user specifies the old option --with-license-file.

For example, before pip-license 1.x to 2.x, the system warns to user if the deprecated option is specified. See sample implementation.

Of course, the idea is to release 3.0 with the --with-license-file option intact. However, the behavior has changed so much that it seems better for users to know that they are now a "different option".

@johnthagen
Copy link
Contributor Author

First, I would like the base branch of PR to be release-3.0 instead of master.

Done.

If you replace a valid option with --with-license-files, you will be warned when a user specifies the old option --with-license-file.

Done.

I think that if there is a single license file found, output it without enclosing it in [].

I implemented this for CSV class. Note that CSV still suffers from \n in the LicenseFile contents themselves.

@raimon49
Copy link
Owner

raimon49 commented Sep 1, 2020

It looks like the base hasn't been changed yet.

You can change it from the Edit button.
image

piplicenses.py Outdated Show resolved Hide resolved
@johnthagen johnthagen changed the base branch from master to release-3.0.0 September 1, 2020 10:50
@johnthagen
Copy link
Contributor Author

It looks like the base hasn't been changed yet.

Ah, got it. I had rebased the fork via Git, but hadn't told GitHub to change the base for viewing purposes on GitHub.

@johnthagen johnthagen marked this pull request as ready for review September 8, 2020 00:02
@johnthagen
Copy link
Contributor Author

@raimon49 Checking in on this. Anything else you want to see before the 3.0.0 release? I noticed Python 3.9 comes out tomorrow.

@raimon49
Copy link
Owner

@johnthagen Thanks for the notification.

I have taken this Pull Request and considered releasing version 3.0.0. But the scope of the changes is wide and some unit tests are broken. In particular, it conflicts with the tests added in #76.

Your idea is great, but I'm releasing version 3.0.0 with no changes in the handling of the license file.

@raimon49 raimon49 closed this Oct 25, 2020
@raimon49 raimon49 deleted the branch raimon49:release-3.0.0 October 25, 2020 01:42
@johnthagen
Copy link
Contributor Author

@raimon49 Is handling multiple license files still something that can be considered? If so, do you know what approach you would accept?

@raimon49
Copy link
Owner

@johnthagen Handling multiple license files is an idea worth considering.

The problem is that the current pip-licenses code and testing is vulnerable to major changes. I will attempt to resolve the problem when I have the time to do so. I may then cherry-pick some commits from your open Pull Request.

@johnthagen
Copy link
Contributor Author

Sounds great! When you get to it, if you'd like any more help shoot me a ping.

@gedalia
Copy link

gedalia commented Nov 30, 2023

I'm guessing this never got addressed?

@stefan6419846
Copy link

AFAIK not for now, as well as some other stuff from the version 4.0 list (which is not correctly updated and some stuff might be biased like a settings file for VS Code or environment caching benefits), although version 4.0 has been released over a year ago.

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

Successfully merging this pull request may close these issues.

Packages with multiple LICENSES files not shown in --with-license-file
4 participants