Make htmlreport installable single package by vpetrigo · Pull Request #6335 · cppcheck-opensource/cppcheck

vpetrigo · 2024-04-23T20:36:53Z

Hello!

I would like to propose a set of changes to make cppcheck-htmlreport installable. That would allow to easliy install that module via pip, pipx, etc.

For example, with pipx you can install cppcheck-htmlreport executable as follow:

pipx install "git+https://github.com/vpetrigo/cppcheck@feature/installable_htmlreport#subdirectory=htmlreport"

That also allows populating Pygments into user's environment automatically. No need to switch to the htmlreport and execute pip install . or something.

After that the user is able to use cppcheck-htmlreport without installation of DEB/RPM packages. Moreover, on Windows it allows usage of this utility withot necessity to execute it like that, because shebans are not supported there:

python <path/to/script>/cppcheck-htmlreport

That changes can be used for publishing the package to the PyPI if needed.

Let me know if you would like me to update/change something in this PR.

Thank you!

firewave · 2024-04-23T21:07:46Z

Thanks for your contribution.

It is has been quite a while since I did a Python package so I am not sure how well I can comment on that. So just some generic observations.

Would this work as intended? We rarely touch this but this would decouple the script from the binary. So usually the versions would be in sync but now you have be running different versions and that might lead to issues in the future. We definitely do not intend to have some sort of backwards compatibility being maintained in the script. And on a non-rolling distro without something brew/universe/AUR the cppcheck version is usually lacking way behind whereas the version in PyPi would be the latest. I am not sure we actually want to publish that.

Please make sure you properly rename cppcheck-htmlreport in git so we do not lose the history. Currently it looks like you just renamed it in the filesystem and git doesn't report it as moved/renamed. (I know that it sometimes just doesn't work - git is weird).

We also need to make sure that it will still work for existing users/installations so we possibly need some kind of compatibility layer/link. That might get in the way of renaming the file though.

You also removed the executable flag from test_htmlreport.py. No idea if that was intentional.

…installation

…tallation

vpetrigo · 2024-04-23T22:02:21Z

@firewave, thank you for the prompt reply!

You also removed the executable flag from test_htmlreport.py. No idea if that was intentional.

That was Windows removing this flag for some reason. Should be fixed now.

Please make sure you properly rename cppcheck-htmlreport in git so we do not lose the history. Currently it looks like you just renamed it in the filesystem and git doesn't report it as moved/renamed. (I know that it sometimes just doesn't work - git is weird).

Should be fixed, thank you for pointing out!

Would this work as intended? We rarely touch this but this would decouple the script from the binary.

With those changes I can install the cppcheck-htmlreport with pip/pipx and it is available as an executable:

cppcheck\htmlreport $ pip install .

Processing d:\projects\python\cppcheck\htmlreport
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Installing backend dependencies ... done
  Preparing metadata (pyproject.toml) ... done
Requirement already satisfied: Pygments in d:\projects\python\cppcheck\venv\lib\site-packages (from cppcheck-htmlreport==0.0.0) (2.17.2)
Building wheels for collected packages: cppcheck-htmlreport
  Building wheel for cppcheck-htmlreport (pyproject.toml) ... done
  Created wheel for cppcheck-htmlreport: filename=cppcheck_htmlreport-0.0.0-py3-none-any.whl size=11757 sha256=b167955971dbe4e7031402012fe2c9dcd5c06d83c0189daa4fe5dccd963c5b02
  Stored in directory: C:\Users\vladi\AppData\Local\Temp\pip-ephem-wheel-cache-nq44u7vz\wheels\42\5c\77\bd3ae971d3662d84cd1ec450ecf4d5bb4dca8e679d08ee5b6c
Successfully built cppcheck-htmlreport
Installing collected packages: cppcheck-htmlreport
Successfully installed cppcheck-htmlreport-0.0.0
# ===========================================================================
cppcheck\htmlreport $ gcm cppcheck-htmlreport

CommandType     Name                                               Version    Source
-----------     ----                                               -------    ------
Application     cppcheck-htmlreport.exe                            0.0.0.0    D:\Projects\python\cppcheck\venv\Scripts\cppcheck-htmlreport.exe

# ===========================================================================

cppcheck\htmlreport $ cppcheck-htmlreport --help   
Usage: cppcheck-htmlreport [options]

Options:
  -h, --help            show this help message and exit
  --title=TITLE         The title of the project.
  --file=FILE           The cppcheck xml output file to read defects from. You can combine results from several xml reports i.e. "--file file1.xml --file file2.xml ..". Default is reading from stdin.   
  --report-dir=REPORT_DIR
                        The directory where the HTML report content is written.
  --source-dir=SOURCE_DIR
                        Base directory where source code files can be found.
  --add-author-information=ADD_AUTHOR_INFORMATION
                        Blame information to include. Adds specified author information. Specify as comma-separated list of either "name", "email", "date" or "n","e","d". Default: "n,e,d"
  --source-encoding=SOURCE_ENCODING
                        Encoding of source code.
  --blame-options=BLAME_OPTIONS
                        [-w, -M] blame options which you can use to get author and author mail  -w --> not including white spaces and returns original author of the line  -M --> not including moving    
                        of lines and returns original author of the line

I am not sure what you mean by

this would decouple the script from the binary

cppcheck-htmlreport is not part of Windows install package for example. On Fedora/CentOS it is also distibuted as a separate package. 🤔

One of the options I see to preserve backward compatibility:

htmlreport/
    └── cppcheck_htmlreport/
        ├── cppcheck_htmlreport.py -> cppcheck-htmlreport
        └── ....
    └── cppcheck-htmlreport # original file

Would you tell if that makes sense for you?

firewave · 2024-04-23T22:32:04Z

cppcheck-htmlreport is not part of Windows install package for example.

That seems like an oversight. Please file a ticket about that.

On Fedora/CentOS it is also distibuted as a separate package.

But those should have a fixed dependency on the cppcheck package with the same version so they are tightly coupled. Having a quick look at the Fedora package confirms that - https://fedora.pkgs.org/39/fedora-x86_64/cppcheck-htmlreport-2.12.1-1.fc39.x86_64.rpm.html.

So if you pull it from PyPi you might get the latest script but have an outdated binary. Although many distros now discourage pulling from PyPi and ask you to install the python-* packages from their repo instead.
Since there is already a regular package in distros installing it form PyPi as well would cause a conflict. So this would mainly be something for local installs.

We should also add a test to the CI to make sure that it works and doesn't break in the future. That should be possible by using a virtualenv and would go into scriptcheck.yml.

vpetrigo · 2024-04-23T23:28:57Z

So if you pull it from PyPi you might get the latest script but have an outdated binary.

PyPI provides package's version support. If you release cppcheck-htmlreport it can have the same version as the main cppcheck binary. So, if an user installs a package from PyPI they is able to provide a constrain, e.g. cppcheck-htmlreport==2.14. Frankly speaking, I do not see an issue here, especially since setup.py provides a way to get version from a version control.

It may be benefitial to provide a way to install report generation tool only. For example, we may have a device that runs analysis processes and publish XML reports as artifacts. A QA engineer may want to create a report from that artifacts, so it seems reasonable that they may want to install only HTML generation tool instead of full-featured CppCheck environment they will not use.

We should also add a test to the CI to make sure that it works and doesn't break in the future. That should be possible by using a virtualenv and would go into scriptcheck.yml.

Sure, will update that.

…ackage version

…ow Python 3.7 support

danmar · 2024-04-24T18:30:19Z

Thanks @vpetrigo and @firewave

We rarely touch this but this would decouple the script from the binary.

that is a very good observation that I did not think about directly. So as far as I see we can make pretty drastic changes to cppcheck binary without affecting the compatibility with cppcheck-htmlreport. We can rename every command line flag for instance. But changing the xml file format would break compatibility with cppcheck-htmlreport.

firewave · 2024-04-24T18:36:31Z

But changing the xml file format would break compatibility with cppcheck-htmlreport.

Yeah - I totally blanked on it just depending on the XML output and not the CLI. I have actually never used it (or at least not for many, many, many years) but only adjusted tests for it.

vpetrigo · 2024-04-24T19:49:47Z

So as far as I see we can make pretty drastic changes to cppcheck binary without affecting the compatibility with cppcheck-htmlreport.

Current changes take the version from the Git. So, if a user installs cppcheck-htmlreport something like that (my fork does not have version tags assigned, so the version is somewhat related to dev):

$ pip install "git+https://github.com/vpetrigo/cppcheck@feature/installable_htmlreport#subdirectory=htmlreport"
$ pip list
Package             Version
------------------- ---------------------
cppcheck-htmlreport 0.1.dev28213+ga111c84
pip                 24.0
Pygments            2.17.2

And it provides an understanding that the current cppcheck-htmlreport script was installed from a repo with cppcheck of that version. If something goes wrong, a user can resinstall it using a newer version for example.

danmar

lgtm.. I know nothing about python packaging though.

vpetrigo · 2024-04-25T18:01:27Z

If it is required, let me know if you want to change cppcheck-htmlreport script versioning for distribution at some point. I will be glad to help.

danmar · 2024-04-26T13:32:48Z

@firewave do you think I can merge?

firewave · 2024-04-26T17:05:02Z

Will take a proper look this weekend.

firewave · 2024-04-30T12:25:52Z

+**/*.egg-info/
+
+# cppcheck-htmlreport auto files
+htmlreport/cppcheck_htmlreport/run.py


Should this be fixates on the root i.e. start with a slash?

Yes, that should be updated!

firewave · 2024-04-30T12:27:52Z

-        "cppcheck-htmlreport",
-    ],
-    install_requires=['Pygments']
+    author="Henrik Nilsson",


@danmar Erm...

yes.. could we change author to "Cppcheck team" perhaps?

Updated that.

firewave · 2024-04-30T12:28:10Z

+    },
+    test_suite="tests",
+    install_requires=["Pygments"],
+    setup_requires=["setuptools>=60", "setuptools-scm>=7.0"],


Why do we need those specific versions? Please add a small comment.

Sure, will add a comment. That is the requirement of the stable version of the setuptool-scm package that allows getting package version from a source control.

Comment was added.

firewave · 2024-04-30T12:36:18Z

@@ -1,12 +1,15 @@
 [tox]
-envlist=py26,py27,py32,pypy
+envlist = py26,py27,py32,pypy,pip


I guess we no longer need the py26 stuff. Also Python 2.7 needs to be officially deprecated (I filed https://trac.cppcheck.net/ticket/12682 about that).

Sure, I can remove that. I have not found any usage of tox though in the CI scripts you have. Would you confirm if it is still in use?

I have do not even have an idea what tox is...

If this is unused we should just remove it.

If we keep the file please make sure it was moved within git. Currently it shows up as deleted/added.

BTW Python 2.7 is deprecated now and the leftovers will be removed after the next release.

It should be moved. At least commit history show it that way:

Not sure why it shows up as removed and created under the "File changed" tab.

I have encountered this before. It seems to be a bug or at least some quirk within git/GitHub - and a bad one. Fortunately it only occurred with unimportant files so far.

It is a known GitHub issue: https://github.com/orgs/community/discussions/43177

And it explains why we have only seen it with "unimportant( i.e. small files). It was kind of my assumption that it happens if there were "too many" changes in a moved file.

firewave · 2024-05-06T12:39:13Z

Sorry for being so pedantic about things (I wouldn't call it thorough) but we have so much cruft in our codebase so it makes sense to to take of that while at it.

Also often external submissions are one-shots and we should get those as proper as possible because applying them otherwise there are further pieces to pick up later on. Possibly by somebody else...and possibly not for years...

firewave · 2024-05-06T21:14:01Z

-          htmlreport/test_htmlreport.py
+          test/tools/htmlreport/test_htmlreport.py
+          cd htmlreport
+          ./check.sh


This still needs to be moved to test/tools/htmlreport

Should be updated.

firewave · 2024-05-06T21:18:05Z

+[testenv:pip]
+setenv = PIP_PACKAGE_TEST=1
+
+[testenv:py26]


This can probably be removed as we no longer specify it above.

But I really think we can just drop the whole file as it is about automated testing and we never used in the repo. If somebody external depends on it they will raise this issue and we can then re-add it and also implement it in our CI.

It was added in 97119b8 and the referenced page in that commit doesn't even exist anymore.

tox.ini removed from the repository.

firewave · 2024-05-07T11:35:47Z

    PYTHON=python
 fi

+PROJECT_ROOT_DIR=$(git rev-parse --show-toplevel)


This won't work if you are running this from a source package without a repo.

In other scripts we use DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" to get the script dir. And then traverse back from that.

Should be fixed now.

firewave · 2024-05-07T11:36:15Z

 ERRORLIST_XML="$REPORT_DIR/errorlist.xml"
 UNMATCHEDSUPPR_XML="$REPORT_DIR/unmatchedSuppr.xml"

-$PYTHON cppcheck-htmlreport --file ../gui/test/data/xmlfiles/xmlreport_v2.xml --title "xml2 test" --report-dir "$REPORT_DIR" --source-dir ../test/


Ew. Glad we moved that.

Should this be also removed from the script?

No - that is totally fine.

I was just commenting how that is a good thing that we got rid of the relative paths.

Switch from git project root to POSIX project root getter

firewave

LGTM. If CI passes we can merge this.

Update test cases where Python expects cppcheck-htmlreport script to be in the same directory

…s resolution

firewave · 2024-05-07T20:47:31Z

Thanks a lot for your contribution. And especially your patience.

vpetrigo added 6 commits April 23, 2024 23:33

htmlreport: cppcheck-htmlreport: Move to a directory to allow binary …

5ca82ea

…installation

htmlreport: cppcheck-htmlreport: Move tests to the tests\ subdirectory

66779f2

htmlreport: Fix test file attributes

da3e12b

misc: Update .gitignore with Python specific folders

68e5b78

htmlreport: cppcheck-htmlreport: Update setup.py to allow package ins…

2ef9515

…tallation

htmlreport: cppcheck-htmlreport: Clean up tox.ini formatting

ea9713d

vpetrigo force-pushed the feature/installable_htmlreport branch from 98c1b17 to ea9713d Compare April 23, 2024 21:38

htmlreport: Return original script to initial location

befb561

htmlreport: Add symbolic link to original script

4dee457

vpetrigo added 4 commits April 24, 2024 03:00

htmlreport: Add version getting from Git

2c5149e

htmlreport: cppcheck-htmlreport: Add environment for testing Pip package

9fe9867

htmlreport: cppcheck-htmlreport: Update tests to use standalone/pip p…

9c3a357

…ackage version

github: ci: Add cppcheck-htmlreport Pip package testing

8feaad5

vpetrigo force-pushed the feature/installable_htmlreport branch from 10b6f2d to 8feaad5 Compare April 24, 2024 00:03

vpetrigo added 5 commits April 24, 2024 02:07

htmlreport: test_htmlreport.py: Clean up

c6e117b

github: scriptcheck.yml: Fix test htmlreport

ec736bd

github: scriptcheck.yml: Fix test htmlreport

0ee8ba3

github: scriptcheck.yml: Add pip updating due to tests complains

c2c15de

htmlreport: Relax scm package version requirements in setup.py to all…

a111c84

…ow Python 3.7 support

danmar reviewed Apr 25, 2024

View reviewed changes

firewave reviewed Apr 30, 2024

View reviewed changes

vpetrigo added 2 commits April 30, 2024 21:36

misc: Update .gitignore after review

cd437ff

htmlreport: Clean up after review

9d0c441

vpetrigo requested a review from firewave May 5, 2024 12:35

tox.ini: Fix moving inside the git

4a3cdb2

vpetrigo force-pushed the feature/installable_htmlreport branch from 80f9c06 to 4a3cdb2 Compare May 6, 2024 13:16

vpetrigo added 3 commits May 6, 2024 15:33

htmlreport: Fix test_htmlreport.py file permission after Win

ee16b78

htmlreport: Move tests to htmlreport directory

1e2e4eb

ci/cd: Update htmlreport checking after moving to subfolder

63f9d67

firewave reviewed May 6, 2024

View reviewed changes

vpetrigo added 3 commits May 7, 2024 12:18

htmlreport: Remove unused tox.ini

37927b6

htmlreport: Move check.sh to test/tools

e2bfdfc

ci/cd: Update check.sh execution location

c1f1c62

firewave reviewed May 7, 2024

View reviewed changes

htmlreport: Updated project root determination

249ae9b

Switch from git project root to POSIX project root getter

firewave self-requested a review May 7, 2024 12:04

firewave approved these changes May 7, 2024

View reviewed changes

vpetrigo added 4 commits May 7, 2024 14:34

htmlreport: Fix check.sh

1e4684d

Update test cases where Python expects cppcheck-htmlreport script to be in the same directory

htmlreport: Fix check.sh

3114541

Update test cases where Python expects cppcheck-htmlreport script to be in the same directory

htmlreport: Fix check.sh with absolute path to suppression rules

1264e18

htmlreport: Fix test_suppressions.txt to allow absolute/relative path…

35422c3

…s resolution

vpetrigo requested a review from danmar May 7, 2024 16:18

firewave merged commit f562ff2 into cppcheck-opensource:main May 7, 2024

Conversation

vpetrigo commented Apr 23, 2024

Uh oh!

firewave commented Apr 23, 2024

Uh oh!

vpetrigo commented Apr 23, 2024

Uh oh!

firewave commented Apr 23, 2024

Uh oh!

vpetrigo commented Apr 23, 2024

Uh oh!

danmar commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

firewave commented Apr 24, 2024

Uh oh!

vpetrigo commented Apr 24, 2024

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

vpetrigo commented Apr 25, 2024

Uh oh!

danmar commented Apr 26, 2024

Uh oh!

firewave commented Apr 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firewave May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firewave commented May 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

danmar commented Apr 24, 2024 •

edited

Loading

firewave May 6, 2024 •

edited

Loading