-
Notifications
You must be signed in to change notification settings - Fork 383
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
reintroduce create_skipfile.py script for gerrit #3008
Conversation
11ca1e0
to
22d1ff5
Compare
tools/skiplist/create_skipfile.py
Outdated
import re | ||
|
||
|
||
def create_skipfile(files_changed, skipfile): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inside the common library there is already a similar function to get the list of changed files:
codechecker/codechecker_common/output/gerrit.py
Lines 107 to 138 in 0be059e
def __get_changed_files(changed_file_path: Union[None, str]) -> List[str]: | |
"""Return a list of changed files. | |
Process the given gerrit changed file object and return a list of | |
file paths which changed. | |
The file can contain some garbage values at start, so we use regex | |
to find a json object. | |
""" | |
changed_files = [] | |
if not changed_file_path or not os.path.exists(changed_file_path): | |
return changed_files | |
with open(changed_file_path, | |
encoding='utf-8', | |
errors='ignore') as changed_file: | |
content = changed_file.read() | |
# The file can contain some garbage values at start, so we use | |
# regex search to find a json object. | |
match = re.search(r'\{[\s\S]*\}', content) | |
if not match: | |
return changed_files | |
for filename in json.loads(match.group(0)): | |
if "/COMMIT_MSG" in filename: | |
continue | |
changed_files.append(filename) | |
return changed_files |
Because you created a module in this PR I recommend you to move this function under this tool so we can use this in two places and we can avoid code duplication:
- you can use this __get_changed_files function to create skip file in this tool.
- and you can use this function in the common module.
tools/skiplist/create_skipfile.py
Outdated
# | ||
# ------------------------------------------------------------------------- | ||
""" | ||
Converts Gerrit Review changed files list to CodeChecker skipfile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the structure of our other tools:
- create a readme in the root directory of the tool which describe what this tool does, how you can use it and maybe an example.
- create a codechecker_skiplist directory which will be a module and you move this file also to this directory. So in the CodeChecker we can import and use it.
- I recommend you to rename this file because from the path (tools/skiplist/create_skipfile.py ) for example I can't figure out that this tool is for gerrit integration. So for example the
gerrit_changed_files_to_skipfile
or something similar would be a better name choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved it under the scripts directory as we discussed this will not be a separate tool yet.
I thought about moving it under the bin directory but in that case the create_commands.py script needs to be further complicated and I'm not sure we want that.
22d1ff5
to
8438ab8
Compare
export CC_SKIPFILE="$WORKSPACE/skipfile" | ||
|
||
# gerrit_changed_files_to_skipfile.py script can be found in the bin directory in the CodeChecker package. | ||
python gerrit_changed_files_to_skipfile.py $CC_CHANGED_FILES $CC_SKIPFILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not we need to use python3 binary here? For example in the Makefiles we are using python3 binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the jenkins environment has only python2?
@@ -0,0 +1,54 @@ | |||
# ------------------------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# ------------------------------------------------------------------------- | |
#!/usr/bin/env python3 | |
# ------------------------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I thought about it but the script is python2/python3 compatible now, with that change we limit it to python3 only do we want that?
Makefile
Outdated
@@ -91,7 +91,10 @@ package_statistics_collector: build_statistics_collector package_dir_structure | |||
cd $(CC_BUILD_DIR) && \ | |||
ln -sf ../lib/python3/codechecker_statistics_collector/cli.py bin/post-process-stats | |||
|
|||
package: package_dir_structure set_git_commit_template package_plist_to_html package_tu_collector package_report_converter package_report_hash package_merge_clang_extdef_mappings package_statistics_collector | |||
package_gerrit_skiplist: | |||
cp -rp scripts/gerrit_changed_files_to_skipfile.py $(CC_BUILD_DIR)/bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we specify the interpreter in the python file below we can copy it without the .py extension so this command can be called easily.
Also other commands in the bin directory doesn't have extension so it would be more consistent and the user doesn't have to call this script through the python binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the same reason I did not this change what I described in my previous answer. I thought about it but the script is python2/python3 compatible now, with that change we limit it to python3 only do we want that?
The script was removed in c1626bb but for the usecase where the user does not want to analyze all the source files in the review it is needed.
8438ab8
to
a7c6314
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The script was removed in c1626bb
but for the usecase where the user does not want to analyze
all the source files in the review it is needed.