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.

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

Multiple features: source path substitution, relative source filename display, custom HTML report title #14

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Multiple features: source path substitution, relative source filename display, custom HTML report title #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdf98f7
Adding support for source path substitution
May 23, 2023
97b9c9e
Adding support for displaying relative source filenames
May 23, 2023
ffa3c23
Adding support for custom title in HTML report
May 23, 2023
bbe4e44
Documenting pyinstaller usage
May 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ ValgrindCI uses the `setuptools` to build its package which can then be installe
> pip install ValgrindCI --no-index -f dist
```

#### Build and package an executable with `pyinstaller`

You can use `pyinstaller` to create a single-file executable binary:

```bash
> pip install pyinstaller
> pyinstaller --onefile --add-data ValgrindCI:ValgrindCI valgrind-ci
> ./dist/valgrind-ci --help
```

Comment on lines +47 to +56
Copy link
Owner

Choose a reason for hiding this comment

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

What is the point of including this piece of documentation ? Which audience is it intended to ?

## How to use

ValgrindCI is a command tool designed to be executed within jobs of your favorite Continuous Integration platform. It parses the XML output of valgrind to provide its services.
Expand Down
46 changes: 45 additions & 1 deletion ValgrindCI/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,32 @@ def main():
"--source-dir",
help="specifies the source directory",
)
parser.add_argument(
"--substitute-path",
action="append",
help="specifies a substitution rule `from:to` for finding source files on disk. example: --substitute-path /foo:/bar",
nargs='?'
)
parser.add_argument(
"--relativize",
action="append",
help="specifies a prefix to remove from displayed source filenames. example: --relativize /foo/bar",
nargs='?'
)
parser.add_argument(
"--relativize-from-substitute-paths",
default=False,
action="store_true",
help="use the `from` values in the substitution rules as prefixes to remove from displayed source filenames",
)
parser.add_argument(
"--output-dir", help="directory where the HTML report will be generated"
)
parser.add_argument(
"--html-report-title",
default="ValgrindCI Report",
help="the title of the generated HTML report"
)
parser.add_argument(
"--summary",
default=False,
Expand Down Expand Up @@ -55,6 +78,27 @@ def main():
data.parse(args.xml_file)
data.set_source_dir(args.source_dir)

if args.substitute_path:
substitute_paths = []
for s in args.substitute_path:
substitute_paths.append({"from": s.split(":")[0], "to": s.split(":")[1] })
data.set_substitute_paths(substitute_paths)
Comment on lines +83 to +85
Copy link
Owner

Choose a reason for hiding this comment

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

s.split(':') is executed twice which is inefficient. Please use local variables instead:

Suggested change
for s in args.substitute_path:
substitute_paths.append({"from": s.split(":")[0], "to": s.split(":")[1] })
data.set_substitute_paths(substitute_paths)
for s in args.substitute_path:
_from, _to = s.split(':')
substitute_paths.append({"from": _from, "to": _to})
data.set_substitute_paths(substitute_paths)


if args.relativize:
prefixes = []
for p in args.relativize:
prefixes.append(p)
data.set_relative_prefixes(prefixes)
Comment on lines +87 to +91
Copy link
Owner

@bcoconni bcoconni May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why are you duplicating the list args.relativize in another list prefixes ? Wouldn't it be simpler to send args.relativize as an argument to data.set_relative_prefixes() ?

Suggested change
if args.relativize:
prefixes = []
for p in args.relativize:
prefixes.append(p)
data.set_relative_prefixes(prefixes)
if args.relativize:
data.set_relative_prefixes(args.relativize)


if args.relativize_from_substitute_paths:
if not args.substitute_path:
print("No substitution paths specified on the command line.")
else:
prefixes = data._relative_prefixes.copy()
for s in data._substitute_paths:
prefixes.append(s.get("from"))
data.set_relative_prefixes(prefixes)

errors_total = data.get_num_errors()
if args.abort_on_errors and errors_total != 0:
print("{} errors reported by Valgrind - Abort".format(errors_total))
Expand All @@ -63,7 +107,7 @@ def main():
if args.output_dir:
renderer = HTMLRenderer(data)
renderer.set_source_dir(args.source_dir)
renderer.render(args.output_dir, args.lines_before, args.lines_after)
renderer.render(args.html_report_title, args.output_dir, args.lines_before, args.lines_after)

if args.number_of_errors:
print("{} errors.".format(errors_total))
Expand Down
4 changes: 2 additions & 2 deletions ValgrindCI/data/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@
<html lang="en">

<head>
<title>Valgrind report</title>
<title>{{ title }}</title>
<link href='valgrind.css' rel='stylesheet' type='text/css'>
</head>

<body>
<div class="global">
<div class="header">
<h1>ValgrindCI Report</h1>
<h1>{{ title }}</h1>
<p><b>{{ num_errors }}</b> errors</p>
</div>
<table>
Expand Down
23 changes: 23 additions & 0 deletions ValgrindCI/parse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ class ValgrindData:
def __init__(self) -> None:
self.errors: List[Error] = []
self._source_dir: Optional[str] = None
self._substitute_paths: Optional[List[dict]] = []
self._relative_prefixes: Optional[List[str]] = []
Comment on lines +121 to +122
Copy link
Owner

Choose a reason for hiding this comment

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

Why are these members declared Optional[] ? It doesn't seem to me they can ever be None...

Suggested change
self._substitute_paths: Optional[List[dict]] = []
self._relative_prefixes: Optional[List[str]] = []
self._substitute_paths: List[dict] = []
self._relative_prefixes: List[str] = []


def parse(self, xml_file: str) -> None:
root = et.parse(xml_file).getroot()
Expand All @@ -130,6 +132,27 @@ def set_source_dir(self, source_dir: Optional[str]) -> None:
else:
self._source_dir = None

def set_substitute_paths(self, substitute_paths: Optional[List[dict]]) -> None:
if substitute_paths is not None:
Copy link
Owner

Choose a reason for hiding this comment

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

Can the parameter substitute_paths ever be None ? Why is substitute_paths of type Optional[} ?

self._substitute_paths = substitute_paths

def set_relative_prefixes(self, relative_prefixes: Optional[str]) -> None:
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't the parameter relative_prefixes be of type List[str] ?

Suggested change
def set_relative_prefixes(self, relative_prefixes: Optional[str]) -> None:
def set_relative_prefixes(self, relative_prefixes: List[str]) -> None:

if relative_prefixes is not None:
Copy link
Owner

Choose a reason for hiding this comment

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

Can the parameter relative_prefixes ever be None?

self._relative_prefixes = relative_prefixes
Copy link
Owner

Choose a reason for hiding this comment

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

Mypy reports an error for this line of code: ValgrindCI/parse.py:141: error: Incompatible types in assignment (expression has type "str", variable has type "Optional[List[str]]") [assignment].

Please fix it.


def substitute_path(self, path: str) -> str:
for s in self._substitute_paths:
Copy link
Owner

Choose a reason for hiding this comment

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

Mypy reports an error for this line of code ValgrindCI/parse.py:144: error: Item "None" of "Optional[List[Dict[Any, Any]]]" has no attribute "__iter__" (not iterable) [union-attr].

Please fix it.

path = path.replace(s.get("from"), s.get("to"))
Copy link
Owner

Choose a reason for hiding this comment

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

Mypy reports 2 errors for this line of code:

  • ValgrindCI/parse.py:145: error: Argument 1 to "replace" of "str" has incompatible type "Optional[Any]"; expected "str" [arg-type]
  • ValgrindCI/parse.py:145: error: Argument 2 to "replace" of "str" has incompatible type "Optional[Any]"; expected "str" [arg-type]

Please fix them

return path

def relativize(self, path: str) -> str:
for p in self._relative_prefixes:
Copy link
Owner

Choose a reason for hiding this comment

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

Mypy reports an error for this line of code: ValgrindCI/parse.py:149: error: Item "None" of "Optional[List[str]]" has no attribute "__iter__" (not iterable) [union-attr]

Please fix it.

if path.startswith(p):
path = path.replace(p, "")
if path.startswith("/"):
path = path[1:]
Comment on lines +151 to +153
Copy link
Owner

@bcoconni bcoconni May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Use òs.path.commonprefix()andos.path.relpath()` which are portable across OSes rather than homemade code:

Suggested change
path = path.replace(p, "")
if path.startswith("/"):
path = path[1:]
if os.path.commonprefix([p, path]) == p:
path = os.path.relpath(path, p)

return path

def get_num_errors(self) -> int:
if self._source_dir is None:
return len(self.errors)
Expand Down
54 changes: 32 additions & 22 deletions ValgrindCI/render.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def set_source_dir(self, source_dir: Optional[str]) -> None:
else:
self._source_dir = None

def render(self, output_dir: str, lines_before: int, lines_after: int) -> None:
def render(self, report_title: str, output_dir: str, lines_before: int, lines_after: int) -> None:
if not os.path.exists(output_dir):
os.makedirs(output_dir)
shutil.copy(
Expand All @@ -71,14 +71,14 @@ def render(self, output_dir: str, lines_before: int, lines_after: int) -> None:
f.write(
self._source_tmpl.render(
num_errors=num_errors,
source_file_name=source_file,
source_file_name=self._data.relativize(source_file),
codelines=lines_of_code,
)
)

summary.append(
{
"filename": source_file,
"filename": self._data.relativize(source_file),
"errors": num_errors,
"link": html_filename,
}
Expand All @@ -88,7 +88,9 @@ def render(self, output_dir: str, lines_before: int, lines_after: int) -> None:
with open(os.path.join(output_dir, "index.html"), "w") as f:
f.write(
self._index_tmpl.render(
source_list=summary, num_errors=total_num_errors
title=report_title,
source_list=summary,
num_errors=total_num_errors
)
)

Expand Down Expand Up @@ -121,13 +123,17 @@ def _extract_error_data(
assert error_line is not None
stack["line"] = error_line - lines_before - 1
stack["error_line"] = lines_before + 1
stack["fileref"] = "{}:{}".format(
frame.get_path(self._source_dir), error_line
)
with open(fullname, "r") as f:
for l, code_line in enumerate(f.readlines()):
if l >= stack["line"] and l <= error_line + lines_after - 1:
stack["code"].append(code_line)
frame_source = frame.get_path(self._source_dir)
frame_source = self._data.relativize(frame_source)
stack["fileref"] = "{}:{}".format(frame_source, error_line)
fullname = self._data.substitute_path(fullname)
try:
with open(fullname, "r", errors="replace") as f:
Copy link
Owner

Choose a reason for hiding this comment

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

What is the point of using errors="replace" when reading a file ?

for l, code_line in enumerate(f.readlines()):
if l >= stack["line"] and l <= error_line + lines_after - 1:
stack["code"].append(code_line)
except OSError as e:
print(f"Warning: cannot read stack data from missing source file: {e.filename}")
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't continue be called when swallowing this exception in order to avoid adding a malformed "stack" to the issue dictionary ?

Suggested change
print(f"Warning: cannot read stack data from missing source file: {e.filename}")
print(f"Warning: cannot read stack data from missing source file: {e.filename}")
continue

issue["stack"].append(stack)
return issue

Expand All @@ -143,16 +149,20 @@ def _extract_data_per_source_file(
else:
filename = source_file

with open(filename, "r") as f:
for l, line in enumerate(f.readlines()):
klass = None
issue = None
if l + 1 in error_lines:
klass = "error"
issue = self._extract_error_data(
src_data, l + 1, lines_before, lines_after
filename = self._data.substitute_path(filename)
try:
with open(filename, "r", errors="replace") as f:
Copy link
Owner

Choose a reason for hiding this comment

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

There again, what is the point of using errors="replace" when reading a file ?

for l, line in enumerate(f.readlines()):
klass = None
issue = None
if l + 1 in error_lines:
klass = "error"
issue = self._extract_error_data(
src_data, l + 1, lines_before, lines_after
)
lines_of_code.append(
{"line": line[:-1], "klass": klass, "issue": issue}
)
lines_of_code.append(
{"line": line[:-1], "klass": klass, "issue": issue}
)
except OSError as e:
print(f"Warning: cannot extract data from missing source file: {e.filename}")
return lines_of_code, len(error_lines)