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.

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

Handle ambigous profile/group parameters #4377

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

noraz31
Copy link
Collaborator

@noraz31 noraz31 commented Oct 29, 2024

Introduce the -e prefix: option, to improve handling of ambigous parameters.
such as "security" which is a checker prefix group and a profile at the same time

@noraz31 noraz31 force-pushed the handle_ambigous_parameters branch 4 times, most recently from 23f9a04 to 92cbeaa Compare October 30, 2024 12:55
@noraz31 noraz31 marked this pull request as draft November 4, 2024 11:57
@noraz31 noraz31 marked this pull request as ready for review November 4, 2024 14:55
@noraz31 noraz31 force-pushed the handle_ambigous_parameters branch from 92cbeaa to 22f992d Compare November 4, 2024 15:11
@dkrupp dkrupp requested a review from cservakt November 5, 2024 14:19
@dkrupp dkrupp added this to the release 6.25.0 milestone Nov 5, 2024
Copy link
Contributor

@vodorok vodorok left a comment

Choose a reason for hiding this comment

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

I feel like this change is a backward incompatible change, and would requires another reviewers opinion.
@dkrupp what is your opinion?

self.set_checker_enabled(checker, enabled)
LOG.error(templ.substitute(entity="profile",
identifier=identifier))
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is exit status 1 the proper exit code in case of configuration errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same applies 10 lines below

Copy link
Member

Choose a reason for hiding this comment

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

I think yes. These are the exit code listed by CodeChecker analyze --help
Exit status

0 - Successful analysis
1 - CodeChecker error
3 - Analysis of at least one translation unit failed
128+signum - Terminating on a fatal signal whose number is signum

@@ -210,23 +212,40 @@ def initialize_checkers(self,
profiles = checker_labels.get_description('profile')
guidelines = checker_labels.occurring_values('guideline')

templ = Template("The ${entity} name '${identifier}' conflicts with a "
"checker(-group) name. Please use -e ${entity}: to "
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be very specific about what conflicts with what. So if you can provide an exact error message instead of ...with a checker(-group)... then please be specific.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's right, AFAIK it is always a checker group name that is conflicting, so I'll correct this.

analyzer/codechecker_analyzer/checkers.py Outdated Show resolved Hide resolved
@@ -216,10 +225,15 @@ def f(checks, checkers):

# Enable "security" profile checkers without "profile:" prefix.
cfg_handler = ClangSA.construct_config_handler(args)
cfg_handler.initialize_checkers(checkers,
[('security', True)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you replacing the security prefix? enabling with sensitive?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After this change is introduced, "-e security" should throw an error as "security" is a profile name that conflicts with a checker group name. However, "-e sensitive" should continue to function as it has previously, because the "sensitive" profile name does not conflict with any checker group names. This is what I intend to test here.

@noraz31 noraz31 force-pushed the handle_ambigous_parameters branch from 22f992d to 1ad9851 Compare November 7, 2024 09:31
@noraz31 noraz31 requested a review from dkrupp as a code owner November 7, 2024 09:31
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

  1. Please fix the description of the initialize_checkers function: instead of "Is considered in this order" in case of name clash, an error is emitted.

  2. Also fix the description of the CodeChecker analyze --help that name clashes are not accepted around line 681 " checkers_opts.add_argument('-e', '--enable',"

analyzer/codechecker_analyzer/analyzers/config_handler.py Outdated Show resolved Hide resolved
analyzer/codechecker_analyzer/cmd/analyze.py Outdated Show resolved Hide resolved
analyzer/codechecker_analyzer/cmd/analyze.py Outdated Show resolved Hide resolved
docs/analyzer/user_guide.md Outdated Show resolved Hide resolved
docs/analyzer/user_guide.md Outdated Show resolved Hide resolved
analyzer/tests/unit/test_checker_handling.py Outdated Show resolved Hide resolved
analyzer/tests/unit/test_checker_handling.py Outdated Show resolved Hide resolved
@noraz31 noraz31 force-pushed the handle_ambigous_parameters branch 2 times, most recently from 4c0a007 to e1bf58f Compare November 7, 2024 15:34
Introduce the -e prefix: option, to improve
handling of ambigous parameters
(such as "security" etc.)
@noraz31 noraz31 force-pushed the handle_ambigous_parameters branch from e1bf58f to 919744e Compare November 7, 2024 15:39
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@vodorok vodorok left a comment

Choose a reason for hiding this comment

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

LGTM

@vodorok vodorok merged commit 9598141 into Ericsson:master Nov 11, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants