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

[fix] Analysis shouldn't fail on non-existing directory #3943

Merged
merged 1 commit into from
Jul 4, 2023

Conversation

bruntib
Copy link
Contributor

@bruntib bruntib commented Jun 22, 2023

When the compilation command database contains the build information of a file in a non-existing directory then the pre-analysis phase of CTU shouldn't fail.

@bruntib bruntib added this to the release 6.22.2 milestone Jun 22, 2023
@bruntib bruntib requested a review from Szelethus June 22, 2023 14:14
@bruntib bruntib requested a review from vodorok as a code owner June 22, 2023 14:14
Copy link
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

When the compilation command database contains the build information of a file in a non-existing directory then the pre-analysis phase of CTU shouldn't fail.

Is that okay though? When can that naturally happen and not be a sign of a user error? I am not a fan on this patch.

analyzer/tests/functional/ctu/test_ctu.py Outdated Show resolved Hide resolved
@bruntib bruntib force-pushed the non_existing_dir_fail branch 2 times, most recently from ae20e63 to 4850392 Compare June 28, 2023 13:39
@bruntib bruntib requested a review from Szelethus June 28, 2023 13:41
Copy link
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

Please add a LOG warning or an error (and test it).

@bruntib bruntib force-pushed the non_existing_dir_fail branch from 4850392 to 97ceeb5 Compare June 30, 2023 08:14
@bruntib bruntib requested a review from Szelethus June 30, 2023 08:14
Copy link
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

Its not your fault, but I'm not thrilled about the reasons behind this patch. If someone runs all their configure and setup scripts inside CodeChecker log, and logs build commands for temporary files, thats a user error.

I understand that we need to bend the knee on this one. But I'd prefer to ease up on the hard error / exception escape (before proper logging is implemented) as little as possible.

When the compilation command database contains the build information of
a file in a non-existing directory then the pre-analysis phase of CTU
shouldn't fail.
@bruntib bruntib force-pushed the non_existing_dir_fail branch from 97ceeb5 to 1aeafb7 Compare July 4, 2023 13:37
@bruntib bruntib requested a review from Szelethus July 4, 2023 13:53
Copy link
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

LGTM! I'm happy with where we landed with this all things considered.

@bruntib bruntib merged commit 097eed2 into Ericsson:master Jul 4, 2023
@bruntib bruntib deleted the non_existing_dir_fail branch July 4, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants