-
Notifications
You must be signed in to change notification settings - Fork 11
make docker: allow overriding $(DOCKER) command (e.g. podman) #220
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #220 +/- ##
=======================================
Coverage 58.45% 58.45%
=======================================
Files 12 12
Lines 1372 1372
Branches 323 323
=======================================
Hits 802 802
Misses 434 434
Partials 136 136 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
(to avoid triggering segfaults for failed asserts; this at least gives abort)
| $(MAKE) -C build/tesseract training-install | ||
|
|
||
| TESSERACT_CONFIG ?= --disable-openmp --disable-shared CXXFLAGS="-g -O2 -fPIC -fno-math-errno -Wall -Wextra -Wpedantic" | ||
| TESSERACT_CONFIG ?= --disable-openmp --disable-shared CXXFLAGS="-g -O2 -fPIC -fno-math-errno -Wall -Wextra -Wpedantic -UNDEBUG" |
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.
@bertsky, it's a bit strange if you add an unrelated commit to a pull request which was already reviewed by yourself.
I am not sure that your change in line 126 is a good idea (debug code adds instructions that increase processing time), and you have not given a proper explanation of why you made it.
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'm afraid your change will do exactly what you wanted to avoid.
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.
see this why we must at least avoid NDEBUG; keeping -g otherwise is a good idea to get symbols when there is a crash
all this is a far cry from being satisfactory (libtesseract must use exceptions in the end!), just less insane (segfault)
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.
So you prefer getting a SIGSEGV because of your change instead of getting an abort() like in the current configuration?
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.
in other places like this we need !defined(NDEBUG) for certain safety checks
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.
So you prefer getting a SIGSEGV because of your change instead of getting an abort() like in the current configuration?
No, segfault is obviously even worse, but I'm in a bind: the place that actually triggers my latest segfault/abort needs !defined(NDEBUG) – so the snippet I showed first behaves different, but is (currently) not as relevant
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.
See sirfz/tesserocr#365 for how I would like this to be solved.
No description provided.