search

git-afsantos / haros

H(igh) A(ssurance) ROS - Static analysis of ROS application code.
MIT License
190 stars 37 forks source link

Minimal-size report exporting #89

Closed max-krichenbauer closed 4 years ago

max-krichenbauer commented 4 years ago

In the last QA dashboard meeting @gavanderhoorn mentioned that the size of HAROS reports may pose a problem when trying to use HAROS in a ROS build farm -> ROS wiki environment. So this PR aims to minimize the report data size without affecting how the HAROS html report is displayed. In detail, the following changes were performed:

(1) Removed font-awesome.css and FontAwesome.otf from harosviz/lib/ folder as they are never used (2) Updated the font files in harosviz/lib/fonts/ to only includ those icons actually used by the report (3) Switched from including 3rd party js libraries to referencing the public versions from vendors directly (4) Added a new optional --minimal-output parameter that suppresses exporting files which are not used in the html report (haros.db and metrics files)

Please see the individual commits for more details.

git-afsantos commented 4 years ago

I appreciate the idea, but I have a few issues with the PR itself.

  1. The PR seems to be addressing multiple issues at once: space optimization, code duplication, function renaming...
  2. Some of the deletions (e.g., icons), will have to be reverted if in a later version there is a need for them.
  3. Removing the JS libraries works for the purposes of the Wiki, but Haros itself should be completely functional offline - which is the reason why I added the libraries in the first place. HTML is just a convenient technology choice I was most familiar with, but it could as easily be implemented in Qt, JavaFx, etc.

While 1 and 2 are passable, I cannot merge 3 to master. Either this feature lives in a Wiki-specific fork, or it must be a behaviour configurable via arguments/settings file.

max-krichenbauer commented 4 years ago

Thank you for reviewing the PR!

About (1), sorry I accidentally included other commits in this branch. While it would be more clean to have it in separate branches, I hope the changes are acceptable.

About (2), (3), I understand. I'll edit the procedure that copies the files to the destination, so it will either copy the normal version or the minimal version (linked html files etc), based on whether --minimal-output is set. Then normal users can have the offline version while wiki users can have the minimized version.

git-afsantos commented 4 years ago

it will either copy the normal version or the minimal version (linked html files etc), based on whether --minimal-output is set.

This would be ideal, although it is a bit of work.

max-krichenbauer commented 4 years ago

Sorry for the delay - I tried to find a way that interferes as little as possible with any future development you want to make on HAROS. This is what I suggest in this commit now:

(1) the folder structure and included files stays exactly the same as it was before

(2) during the harosviz installation, if --minimial-report is set, a list of known replacements is checked. Every known replacement is rewritten as external link in index.html and the js file is removed from the report.

I did not change the fonts for now, because they work offline as well and the original fonts are still in the docs/ folder. Instead, I added documentation to the docs/lib/fonts/ folder on how to generate the reduced fonts.

git-afsantos commented 4 years ago

Sorry for the delay, I finally found enough time to review this. Merged. :+1: