Avoid plist filenames being the same #3588

milanlakhani · 2022-01-31T14:13:35Z

This makes the filenames of plist format reports generated by
report-converter include the names of the parent and grandparent
directories as well as the filename, since before if two different
plist files had the same name there was only one of the reports in
the database. Fixes #3436 .

whisperity · 2022-01-31T14:33:04Z

tools/report-converter/codechecker_report_converter/analyzers/analyzer_result.py

@@ -147,7 +147,7 @@ def _write(

        analyzer_info = AnalyzerInfo(name=self.TOOL_NAME)
        for file_path, file_reports in file_to_report.items():
-            source_file = os.path.basename(file_path)
+            source_file = ("-".join(file_path.split("/")[-3:]))


This feels like too much of a symptom fix and a band-aid, which just begs to be broken sometime later by someone whose directories only differ 3 or 4 hops up the chain.

I'm not sure at a glance how hot this path is, but maybe instead we should put a sort of hash of the full file path into the file name?

(At least that is what clangd does with its cache: Something.cpp.AAAAAAAAAA.idx is the file they create, where AAAA... is a hash of the configuration that produced the file (so it includes not only path, but a hash of the compile command too, as far as I know.)

I agree with @whisperity, this feels like a hacky solution which also not always works.

Now the report-converter tool has a --filename option which handles two special values: {source_file} and {analyzer}. My recommendation is to create another special value (e.g.: {hash}) which will insert a hash into the file name.
You can create the file hash similarly to this:

codechecker/analyzer/codechecker_analyzer/analyzers/result_handler_base.py

Line 132 in 2c49428

hashlib.md5(build_info.encode(errors='ignore')).hexdigest()

And the user will be able to use it like this: report-converter --filename "{source_file}_{analyzer}_{hash}"

Also it would be good to add a test case for this use case too to test that report-converter will create multiple plist files not just one.

csordasmarton · 2022-02-01T08:56:48Z

tools/report-converter/codechecker_report_converter/analyzers/analyzer_result.py

@@ -147,7 +147,7 @@ def _write(

        analyzer_info = AnalyzerInfo(name=self.TOOL_NAME)
        for file_path, file_reports in file_to_report.items():
-            source_file = os.path.basename(file_path)
+            source_file = ("-".join(file_path.split("/")[-3:]))


I agree with @whisperity, this feels like a hacky solution which also not always works.

Now the report-converter tool has a --filename option which handles two special values: {source_file} and {analyzer}. My recommendation is to create another special value (e.g.: {hash}) which will insert a hash into the file name.
You can create the file hash similarly to this:

codechecker/analyzer/codechecker_analyzer/analyzers/result_handler_base.py

Line 132 in 2c49428

hashlib.md5(build_info.encode(errors='ignore')).hexdigest()

And the user will be able to use it like this: report-converter --filename "{source_file}_{analyzer}_{hash}"

Also it would be good to add a test case for this use case too to test that report-converter will create multiple plist files not just one.

milanlakhani · 2022-02-01T20:03:08Z

Just reworked the commit to use a file hash, hope this does the job? I'm happy to write a test, does anyone know where I can do this since the test suite is so big?
Thanks,
Milan

tools/report-converter/codechecker_report_converter/cli.py

analyzer/codechecker_analyzer/makefile.py

tools/report-converter/codechecker_report_converter/analyzers/analyzer_result.py

milanlakhani · 2022-02-02T12:00:16Z

Just implemented all the comments, let me know if I need to update any more.

csordasmarton

I have two more tiny comments, otherwise LGTM!

tools/report-converter/tests/functional/cmdline/test_cmdline.py

csordasmarton · 2022-02-03T10:04:24Z

tools/report-converter/codechecker_report_converter/analyzers/analyzer_result.py

@@ -148,10 +149,13 @@ def _write(
        analyzer_info = AnalyzerInfo(name=self.TOOL_NAME)
        for file_path, file_reports in file_to_report.items():
            source_file = os.path.basename(file_path)
+            file_hash = hashlib.md5(file_path.encode(errors='ignore')) \


It would be useful to normalize the file path above when we put it into the dictionary here:

codechecker/tools/report-converter/codechecker_report_converter/analyzers/analyzer_result.py

Line 146 in fc3d1fe

file_to_report[report.file.original_path].append(report)

Change this line to this:

file_path = os.path.normpath(report.file.original_path) file_to_report[file_path].append(report)

This way if we have a/b/../x.cpp and a/x.cpp it will not treat it as two different file paths and will generate two plist files but only one.

I'm not clear on this concept there may be a typo in the last line but I made the change

@milanlakhani In which line do you think there is a typo?

'it will not treat it as two different file paths and will generate two plist files but only one.' @csordasmarton

two plist files but one filepath? i don't see the advantage

milanlakhani · 2022-02-03T15:03:18Z

@csordasmarton thanks for your help! Makes sense to test equal to 2. Made those changes

csordasmarton

Some github actions are failed. Can you please fix it?

Also thank you very much for your hard work on this patch 😊

tools/report-converter/codechecker_report_converter/cli.py

This makes the filenames of plist format reports generated by report-converter include the file hash as well as the file name, since before if two different plist files had the same name there was only one of the reports in the database. Fixes Ericsson#3436 .

tools/report-converter/tests/functional/cmdline/test_cmdline.py

Add test to check that report-converter creates a pfile for each file that a bug is found in.

milanlakhani · 2022-02-03T18:57:33Z

Hi Marton, looks like the trivial bugs are done. Do you have any idea on how we can fix those 2 tests? :)

csordasmarton · 2022-02-04T07:46:47Z

@milanlakhani The problem is that now there is a unique hash in the plist file names produced by the report_converter but the test cases are expect no hashes in the file names. This is the reason why the test cases are failing.

To fix the test cases I recommend to change every .transform function call in the test cases and override the default file_name parameter.

For example change these lines:

codechecker/tools/report-converter/tests/unit/analyzers/test_asan_parser.py

Lines 54 to 55 in 3246665

    
           self.analyzer_result.transform( 
        
               'asan.out', self.cc_result_dir, plist.EXTENSION)

to this:

        self.analyzer_result.transform(
            'asan.out', self.cc_result_dir, plist.EXTENSION,
            file_name="{source_file}_{analyzer}")

And please try to run the test cases locally before you push it again so you can fix every test cases in one shot.

You can run test cases locally with the following commands:

Only the unit test cases: make -C tools/report-converter/ test_unit
Only the functional test cases: make -C tools/report-converter/ test_functional
All the test cases: make -C tools/report-converter/ test

The tests are made to expect filenames with no hashes, so this removes the hashes of the filepaths from the file name and makes the file name just consist of {source_file}_{analyzer}.

milanlakhani · 2022-02-05T10:19:17Z

I tried testing locally and I got a fail in 1/1 test 0.03 seconds with a syntax error in def get_reports(self, file_path: str) -> List[Report]: in tools/report-converter/codechecker_report_converter/analyzers/sanitizers/address/analyzer_result.py

milanlakhani · 2022-02-06T21:07:10Z

Thanks for all the help @csordasmarton

csordasmarton

@milanlakhani It works perfectly 😊 Thank you very much for your patch and your hard work on it 😊

milanlakhani requested a review from csordasmarton as a code owner January 31, 2022 14:13

whisperity reviewed Jan 31, 2022

View reviewed changes

csordasmarton suggested changes Feb 1, 2022

View reviewed changes

milanlakhani force-pushed the lano/reportconverter-files branch from 246dc90 to 29753c3 Compare February 1, 2022 19:57

milanlakhani requested review from bruntib and dkrupp as code owners February 1, 2022 19:57

csordasmarton suggested changes Feb 2, 2022

View reviewed changes

tools/report-converter/codechecker_report_converter/cli.py Outdated Show resolved Hide resolved

analyzer/codechecker_analyzer/makefile.py Outdated Show resolved Hide resolved

tools/report-converter/codechecker_report_converter/analyzers/analyzer_result.py Show resolved Hide resolved

milanlakhani force-pushed the lano/reportconverter-files branch from 29753c3 to 999446b Compare February 2, 2022 11:50

milanlakhani force-pushed the lano/reportconverter-files branch from 999446b to 29149b1 Compare February 2, 2022 12:02

csordasmarton suggested changes Feb 3, 2022

View reviewed changes

csordasmarton added this to the release 6.19.0 milestone Feb 3, 2022

csordasmarton added bugfix 🔨 tools 🛠️ Meta-tag for all the additional tools supplied with CodeChecker: plist2html, tu_collector, etc. labels Feb 3, 2022

milanlakhani force-pushed the lano/reportconverter-files branch from 29149b1 to 09e660f Compare February 3, 2022 15:01

csordasmarton suggested changes Feb 3, 2022

View reviewed changes

tools/report-converter/codechecker_report_converter/cli.py Outdated Show resolved Hide resolved

milanlakhani force-pushed the lano/reportconverter-files branch from 09e660f to d43bdb0 Compare February 3, 2022 15:28

csordasmarton reviewed Feb 3, 2022

View reviewed changes

tools/report-converter/tests/functional/cmdline/test_cmdline.py Show resolved Hide resolved

Add test to check pfile creation

9a194a3

Add test to check that report-converter creates a pfile for each file that a bug is found in.

milanlakhani force-pushed the lano/reportconverter-files branch from d43bdb0 to 9a194a3 Compare February 3, 2022 18:48

Transform filenames in tests to remove filehashes

1c36d2f

The tests are made to expect filenames with no hashes, so this removes the hashes of the filepaths from the file name and makes the file name just consist of {source_file}_{analyzer}.

milanlakhani force-pushed the lano/reportconverter-files branch from 716620b to 1c36d2f Compare February 5, 2022 10:07

csordasmarton approved these changes Feb 7, 2022

View reviewed changes

csordasmarton merged commit 9004c10 into Ericsson:master Feb 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid plist filenames being the same #3588

Avoid plist filenames being the same #3588

milanlakhani commented Jan 31, 2022

whisperity Jan 31, 2022

csordasmarton Feb 1, 2022

csordasmarton Feb 1, 2022

milanlakhani commented Feb 1, 2022

milanlakhani commented Feb 2, 2022 •

edited

Loading

csordasmarton left a comment

csordasmarton Feb 3, 2022

milanlakhani Feb 3, 2022

csordasmarton Feb 3, 2022

milanlakhani Feb 3, 2022 •

edited

Loading

milanlakhani Feb 3, 2022

milanlakhani commented Feb 3, 2022

csordasmarton left a comment

milanlakhani commented Feb 3, 2022

csordasmarton commented Feb 4, 2022

milanlakhani commented Feb 5, 2022

milanlakhani commented Feb 6, 2022 •

edited

Loading

csordasmarton left a comment •

edited

Loading

Avoid plist filenames being the same #3588

Avoid plist filenames being the same #3588

Conversation

milanlakhani commented Jan 31, 2022

whisperity Jan 31, 2022

Choose a reason for hiding this comment

csordasmarton Feb 1, 2022

Choose a reason for hiding this comment

csordasmarton Feb 1, 2022

Choose a reason for hiding this comment

milanlakhani commented Feb 1, 2022

milanlakhani commented Feb 2, 2022 • edited Loading

csordasmarton left a comment

Choose a reason for hiding this comment

csordasmarton Feb 3, 2022

Choose a reason for hiding this comment

milanlakhani Feb 3, 2022

Choose a reason for hiding this comment

csordasmarton Feb 3, 2022

Choose a reason for hiding this comment

milanlakhani Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

milanlakhani Feb 3, 2022

Choose a reason for hiding this comment

milanlakhani commented Feb 3, 2022

csordasmarton left a comment

Choose a reason for hiding this comment

milanlakhani commented Feb 3, 2022

csordasmarton commented Feb 4, 2022

milanlakhani commented Feb 5, 2022

milanlakhani commented Feb 6, 2022 • edited Loading

csordasmarton left a comment • edited Loading

Choose a reason for hiding this comment

milanlakhani commented Feb 2, 2022 •

edited

Loading

milanlakhani Feb 3, 2022 •

edited

Loading

milanlakhani commented Feb 6, 2022 •

edited

Loading

csordasmarton left a comment •

edited

Loading