Move all public headers to include/tesseract #2735

amitdo · 2019-10-28T19:34:59Z

Also:

Fix #include path of public headers
Fix autotools build

stweil · 2019-10-28T19:42:42Z

.gitignore

@@ -36,7 +36,7 @@ config_auto.h
 /doc/*.xml

 # generated version file
-/src/api/tess_version.h
+/include/tesseract/tess_version.h


The file name is now always preceded by the tesseract directory, so we could simply use tesseract/version.h.

amitdo · 2019-10-28T19:46:39Z

This does not include the needed changes to CMake and sw, so the CI will fail.

I tested it with autotools and it works fine.

@egorpugin, please push more commits to this PR or first merge and do the CMake fix later.

Please don't squash my commits.

amitdo · 2019-10-28T19:52:54Z

See #594 (comment)

lgtm-com · 2019-10-28T20:15:07Z

This pull request fixes 1 alert when merging 2f8884a into cede5b3 - view on LGTM.com

fixed alerts:

1 for Comparison result is always the same

egorpugin · 2019-10-28T20:28:48Z

@stweil @zdenop
Is this ok for you?
Waiting for the approval/confirmation for such changes.

stweil · 2019-10-28T20:35:55Z

include/tesseract/Makefile.am

@@ -0,0 +1,54 @@
+AM_CPPFLAGS += \


Is AM_CPPFLAGS really needed here?

Not sure. You can try to remove it and see if it compiles.
Remove (usr/local)/include/tesseract in your system first.

It works without it because no files are compiled there. But I suggest an even more radical modification, namely no include/tesseract/Makefile.am at all. Instead use this code:

diff --git a/Makefile.am b/Makefile.am index 27de8e24..1e900be8 100644 --- a/Makefile.am +++ b/Makefile.am @@ -22,7 +22,7 @@ endif SUBDIRS = src/arch src/ccutil src/viewer src/cutil src/opencl src/ccstruct SUBDIRS += src/dict src/classify src/wordrec src/textord src/lstm -SUBDIRS += src/ccmain src/api . tessdata doc unittest include/tesseract +SUBDIRS += src/ccmain src/api . tessdata doc unittest EXTRA_DIST = README.md LICENSE EXTRA_DIST += aclocal.m4 config configure.ac autogen.sh @@ -65,6 +65,25 @@ doc-clean: pkgconfigdir = $(libdir)/pkgconfig pkgconfig_DATA = tesseract.pc +pkginclude_HEADERS = $(top_builddir)/include/tesseract/tess_version.h +pkginclude_HEADERS += include/tesseract/apitypes.h +pkginclude_HEADERS += include/tesseract/baseapi.h +pkginclude_HEADERS += include/tesseract/capi.h +pkginclude_HEADERS += include/tesseract/genericvector.h +pkginclude_HEADERS += include/tesseract/helpers.h +pkginclude_HEADERS += include/tesseract/ltrresultiterator.h +pkginclude_HEADERS += include/tesseract/ocrclass.h +pkginclude_HEADERS += include/tesseract/osdetect.h +pkginclude_HEADERS += include/tesseract/pageiterator.h +pkginclude_HEADERS += include/tesseract/platform.h +pkginclude_HEADERS += include/tesseract/publictypes.h +pkginclude_HEADERS += include/tesseract/renderer.h +pkginclude_HEADERS += include/tesseract/resultiterator.h +pkginclude_HEADERS += include/tesseract/serialis.h +pkginclude_HEADERS += include/tesseract/strngs.h +pkginclude_HEADERS += include/tesseract/thresholder.h +pkginclude_HEADERS += include/tesseract/unichar.h + # fuzzer-api is used for fuzzing tests. # They are run by OSS-Fuzz https://oss-fuzz.com/, but can also be run locally. # Note: -fsanitize=fuzzer currently requires the clang++ compiler.

Yes, I agree.

src/ccutil/Makefile.am

stweil

unittest also needs fixed include statements.
The include statements in include/tesseract don't need the added tesseract/.

amitdo · 2019-10-28T21:10:28Z

unittest also needs fixed include statements.

e1bae15#diff-ee6e88ad125acd6b96556b3373ed48d7

amitdo · 2019-10-28T21:16:10Z

This pull request fixes 1 alert when merging 2f8884a into cede5b3 - view on LGTM.com

fixed alerts:

1 for Comparison result is always the same

I have no clue how this PR fixed that alert :-)

stweil · 2019-10-28T21:18:45Z

unittest also needs fixed include statements.

e1bae15#diff-ee6e88ad125acd6b96556b3373ed48d7

What about unittest/applybox_test.cc (and some more)?

amitdo · 2019-10-28T21:36:12Z

The include statements in include/tesseract don't need the added tesseract/.

At least some Google projects do what I did. I'm not sure what's the right thing to do here.
https://github.com/google/leveldb/blob/master/include/leveldb/cache.h#L23

amitdo · 2019-10-28T21:44:29Z

Another example:
https://github.com/apache/mesos/blob/master/include/mesos/executor.hpp#L24

amitdo · 2019-10-28T21:51:51Z

What about unittest/applybox_test.cc (and some more)?

My hacky python script searches for files with *.cpp or *.h.

That's why it missed *.cc ...

lgtm-com · 2019-10-28T22:21:08Z

This pull request fixes 1 alert when merging d390bfd into cede5b3 - view on LGTM.com

fixed alerts:

1 for Comparison result is always the same

zdenop · 2019-10-29T06:53:42Z

I am fine with this change.
Also there is a need to move tesseractmain.cpp from api directory as it is not part of API (to progs?)...

stweil · 2019-10-29T07:06:56Z

Also there is a need to move tesseractmain.cpp from api directory as it is not part of API (to progs?)...

I would not mix this with the current pull request. One could also argue that tesseractmain.cpp is an example of using the API, so it can stay there.

stweil · 2019-10-29T07:16:07Z

The include statements in include/tesseract don't need the added tesseract/.

At least some Google projects do what I did. I'm not sure what's the right thing to do here.
https://github.com/google/leveldb/blob/master/include/leveldb/cache.h#L23

That are more examples of recursive Makefiles. They are not wrong, but they have several disadvantages. Parallel builds using more than one CPU are much slower with recursions. Additional recursive make processes need more RAM.

That's why I would like to move the autoconf builds away from recursive builds. The cmake and sw builds already use a flat build model.

zdenop · 2019-10-29T07:22:21Z

I agree not to include it to this PR, but when we are "moving sources", it would be nice to make all known changes "together", so we could have stable source tree for some time ;-). IMO using examples instead of progs for tesseractmain.cpp would be misleading because tesseract executable is needed for training.

Maybe it would make sense to move tesseract library to separate repository and include it to tesseract repository as submodule. This should make easier to include tesseact library to other projects (e.g. wrappers)

Signed-off-by: Stefan Weil <sw@weilnetz.de>

lgtm-com · 2019-10-29T08:27:58Z

This pull request fixes 1 alert when merging 26ba7e2 into cede5b3 - view on LGTM.com

fixed alerts:

1 for Comparison result is always the same

stweil · 2019-10-29T08:28:49Z

I fixed Travis CI and the unittest code now, but Appveyor CI still fails. @egorpugin, @zdenop, can we merge the pull request nevertheless and fix cppan.yml and sw.cpp later? Other changes to be done later:

Rename tesseract/tess_version.h to tesseract/version.h
Use flat autoconf make for include/tesseract instead of recursive make
Maybe replace #include "tesseract/*.h" by #include <tesseract/*.h>

amitdo · 2019-10-29T08:46:38Z

#594
Egor suggested to move the programs to 'tools', 'tools/training'.

About a non-recursive make build:
#974

egorpugin · 2019-10-29T10:21:39Z

+1 for Makefile.am removal from include/tesseract

amitdo added 3 commits October 28, 2019 18:50

Move all public headers to include/tesseract

dfede8a

Fix #include path of public headers

e1bae15

Fix autotools build

2f8884a

stweil reviewed Oct 28, 2019

View reviewed changes

src/ccutil/Makefile.am Show resolved Hide resolved

stweil requested changes Oct 28, 2019

View reviewed changes

Support cmake with new include file structure

17b0951

Signed-off-by: Stefan Weil <sw@weilnetz.de>

stweil force-pushed the move-headers2 branch from d390bfd to 17b0951 Compare October 29, 2019 07:35

Fix #include path of public headers for unittest

26ba7e2

Signed-off-by: Stefan Weil <sw@weilnetz.de>

egorpugin merged commit aeb98d9 into tesseract-ocr:master Oct 29, 2019

amitdo deleted the move-headers2 branch October 29, 2019 09:49

stweil mentioned this pull request Nov 26, 2019

automake: Use non-recursive build for tesseract and libtesseract #2785

Merged

amitdo added the modernize label Mar 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all public headers to include/tesseract #2735

Move all public headers to include/tesseract #2735

amitdo commented Oct 28, 2019

stweil Oct 28, 2019

amitdo Oct 28, 2019

amitdo commented Oct 28, 2019

amitdo commented Oct 28, 2019

lgtm-com bot commented Oct 28, 2019

egorpugin commented Oct 28, 2019

stweil Oct 28, 2019

amitdo Oct 28, 2019

stweil Oct 28, 2019 •

edited

Loading

amitdo Oct 28, 2019

stweil left a comment

amitdo commented Oct 28, 2019

amitdo commented Oct 28, 2019 •

edited

Loading

stweil commented Oct 28, 2019

amitdo commented Oct 28, 2019 •

edited

Loading

amitdo commented Oct 28, 2019

amitdo commented Oct 28, 2019 •

edited

Loading

lgtm-com bot commented Oct 28, 2019

zdenop commented Oct 29, 2019

stweil commented Oct 29, 2019

stweil commented Oct 29, 2019 •

edited

Loading

zdenop commented Oct 29, 2019

lgtm-com bot commented Oct 29, 2019

stweil commented Oct 29, 2019

amitdo commented Oct 29, 2019

egorpugin commented Oct 29, 2019

Move all public headers to include/tesseract #2735

Move all public headers to include/tesseract #2735

Conversation

amitdo commented Oct 28, 2019

stweil Oct 28, 2019

Choose a reason for hiding this comment

amitdo Oct 28, 2019

Choose a reason for hiding this comment

amitdo commented Oct 28, 2019

amitdo commented Oct 28, 2019

lgtm-com bot commented Oct 28, 2019

egorpugin commented Oct 28, 2019

stweil Oct 28, 2019

Choose a reason for hiding this comment

amitdo Oct 28, 2019

Choose a reason for hiding this comment

stweil Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

amitdo Oct 28, 2019

Choose a reason for hiding this comment

stweil left a comment

Choose a reason for hiding this comment

amitdo commented Oct 28, 2019

amitdo commented Oct 28, 2019 • edited Loading

stweil commented Oct 28, 2019

amitdo commented Oct 28, 2019 • edited Loading

amitdo commented Oct 28, 2019

amitdo commented Oct 28, 2019 • edited Loading

lgtm-com bot commented Oct 28, 2019

zdenop commented Oct 29, 2019

stweil commented Oct 29, 2019

stweil commented Oct 29, 2019 • edited Loading

zdenop commented Oct 29, 2019

lgtm-com bot commented Oct 29, 2019

stweil commented Oct 29, 2019

amitdo commented Oct 29, 2019

egorpugin commented Oct 29, 2019

stweil Oct 28, 2019 •

edited

Loading

amitdo commented Oct 28, 2019 •

edited

Loading

amitdo commented Oct 28, 2019 •

edited

Loading

amitdo commented Oct 28, 2019 •

edited

Loading

stweil commented Oct 29, 2019 •

edited

Loading