-
Notifications
You must be signed in to change notification settings - Fork 107
Tools for analyzing the compilation on older toolchains #2556
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
base: master
Are you sure you want to change the base?
Conversation
…ains. Signed-off-by: Johannes Kalmbach <[email protected]>
Signed-off-by: Johannes Kalmbach <[email protected]>
| libboost-container1.81-dev \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| # Install GCC-8 by temporarily enabling the Focal (20.04) repositories |
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.
This will break 2032 at latest when focal is completely dropped from Ubuntu's mirrors :D
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 am fine with that. Until then we can probably also drop the support for C++17...
| @@ -0,0 +1,70 @@ | |||
| # Dockerfile for building QLever with GCC8 for C++17 compatibility testing | |||
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 you plan on incorporating the additions from this PR into the automated checks?
If so, since the code for actually compiling qlever is not part of the Dockerfiles, you should publish the docker container images to a package repo like Docker hub oder Github packages and use them directly instead of rebuilding the entire container image each time (and, in particular, downloading all the dependencies each time)
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.
We have an automated check already in place, which installs the dependencies directly. This PR only adds tools to analyze the builds locally. Thank you for your feedback though, we can maybe use that technique for more efficient CI checks in general in the future (not now though).
| echo " python3 /qlever/misc/gcc8_logs_analyzer.py ${BUILD_LOG}" | ||
| echo "" | ||
| echo "Or examine the build log directly:" | ||
| echo " less ${BUILD_LOG}" |
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.
| echo " less ${BUILD_LOG}" | |
| echo " less ${BUILD_LOG}" | |
| exit $BUILD_EXIT_CODE |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2556 +/- ##
==========================================
+ Coverage 91.13% 91.14% +0.01%
==========================================
Files 467 467
Lines 39937 39939 +2
Branches 5351 5351
==========================================
+ Hits 36397 36404 +7
Misses 2014 2014
+ Partials 1526 1521 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Overview
Conformance check passed ✅No test result changes. |
RobinTF
left a comment
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.
Not much to add from me.
| builder.add_annotation(file=err['filename'], line=err['line'], col=err['column'], message=err['message']) | ||
| builder.end_group(collapse=True) | ||
|
|
||
| # Used but never defined errors |
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.
The comments are kinda just repeating the line below.
|
|
||
| # No member named errors | ||
| if no_member_named_errors: | ||
| builder.begin_group(f"'no member named' Errors: {len(no_member_named_errors)}", collapse=True) |
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 know this just repeats the pattern, but spelling "errors" with uppercase E seems odd to me here.
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.
this comes from a BMW dev, I will stay with the current pattern for now.
|



Those include