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

Refactor Autotools build #974

Closed
wants to merge 4 commits into from
Closed

Refactor Autotools build #974

wants to merge 4 commits into from

Conversation

amitdo
Copy link
Collaborator

@amitdo amitdo commented Jun 3, 2017

Use non-recursive make.

@amitdo
Copy link
Collaborator Author

amitdo commented Jun 3, 2017

DO NOT MERGE YET.

This needs a review and testing of the Autotools build on all platforms including training tools and various configure options.

@amitdo
Copy link
Collaborator Author

amitdo commented Jun 4, 2017

@stweil, can you review and test it?

@amitdo
Copy link
Collaborator Author

amitdo commented Jun 6, 2017

@stweil, I asked you to test it, not to break it... :-)

@stweil
Copy link
Member

stweil commented Jun 6, 2017

I'm sorry. That breakage was not intentional of course. With 16 files involved, chances for a conflict are not so low.

As I am not an expert for autotools, I wonder what could be the advantage of a non-recursive make. Do you have some documentation or examples how projects like Tesseract should use autoconf / automake?

@amitdo
Copy link
Collaborator Author

amitdo commented Jun 6, 2017

The main advantage is that IMO it is much more readable and easier to understand in that way.

Also, I fixed the non-working as intended debug-mode.

@amitdo
Copy link
Collaborator Author

amitdo commented Jun 6, 2017

include api/Makefile.am
include arch/Makefile.am
include ccmain/Makefile.am
include ccstruct/Makefile.am
include ccutil/Makefile.am
include classify/Makefile.am
include cutil/Makefile.am
include dict/Makefile.am
include lstm/Makefile.am
include opencl/Makefile.am
include textord/Makefile.am
include viewer/Makefile.am
include wordrec/Makefile.am

All these am files now include only list of source files.

The include here is like C/C++ #include - it is equivalent to pasting the content of the included files in the file that includes them.

It's possible to just put the list of source files in the top Makefile.am, or to include just one file that will contain the list of source files.

@stweil
Copy link
Member

stweil commented Jun 7, 2017

So all compilations are now using the same compiler flags? That has some disadvantages (unnecessary long include paths, unclear dependencies) and breaks portability of the generated binaries: if all files are compiled with AVX support, the resulting code will crash on CPUs without AVX.

@amitdo
Copy link
Collaborator Author

amitdo commented Jun 7, 2017

So all compilations are now using the same compiler flags?

All the library files are using the same flags. I see no problem with that.
I believe that's the norm with projects that use other build system like CMake.

If all files are compiled with AVX support, the resulting code will crash on CPUs without AVX.

I don't think so. The 'logic' of the build is in the top Makefile.am. The SIMD flags are still added conditionally.

@stweil
Copy link
Member

stweil commented Jun 7, 2017

The condition is based on compiler support, so as soon as my compiler supports AVX or SSE 4.1, those flags are enabled, no matter what my CPU supports. Now they are used for each compilation. The compiler will generate code with AVX or SSE instructions. That code will crash on a host without AVX or SSE.

I made a branch with your patch at https://github.com/stweil/tesseract/tree/nonrecursivemake and resolved the conflicts.

@stweil
Copy link
Member

stweil commented Jun 7, 2017

I can confirm that the debug mode uses -O0 as intended (instead of -O2) with this patch.

@amitdo
Copy link
Collaborator Author

amitdo commented Jun 7, 2017

If what you say is true, there will be a crash with the current implementation too.

@amitdo
Copy link
Collaborator Author

amitdo commented Jun 7, 2017

CC: @jbreiden (In your Debian/Ubuntu maintainer hat)

@amitdo
Copy link
Collaborator Author

amitdo commented Jun 7, 2017

@stweil , I'm sorry about the typo in your name in the commit message!

@amitdo
Copy link
Collaborator Author

amitdo commented Nov 7, 2019

@stweil, you seem to like the non-recursive (auto)make idea now.

Should I send a new PR?

The arch directory will still build an internal automake library that has the simd flags. This should fix the issue you raised here.

@stweil
Copy link
Member

stweil commented Nov 7, 2019

Indeed, I now see the advantages of using non-recursive builds.

Maybe you can start with a PR which eliminates a single subdirectory, for example src/api. Then the review is easier, and we can nevertheless discuss whether the principal changes are fine or whether something should be made differently. One example:

I think that code like this:

NAME := n1
NAME += n2
NAME += n3

is easier to maintain than

NAME := n1 \
  n2 \
  n3

@amitdo
Copy link
Collaborator Author

amitdo commented Nov 7, 2019

Maybe you can start with a PR which eliminates a single subdirectory, for example src/api. Then the review is easier, and we can nevertheless discuss whether the principal changes are fine or whether something should be made differently. One example:

I'll do it.

I'll choose the style you like, shown in your comment.

@amitdo
Copy link
Collaborator Author

amitdo commented Nov 7, 2019

There's another thing I want to ask you.

Here are 4 ways I can organize this task.

  1. Put all flags and sources in the top Makefile.am.
  2. Put all flags in the top Makefile.am. The list of source files will stay in subdirs.
    The top Makefile.am will use 'include...' statements.
    See Refactor Autotools build #974 (comment)
  3. Put all flags in the top Makefile.am. The complete list of source files will be in a separate file: Sources.am. No am files in subdirs.
  4. Same as 3, but two files for the source list: Sources.am and SourcesLegacy.am.

In any case, arch, training and unittest will stay the same as they are now.

All these options will have the same efficiency. All of them will be non-recursive.

What's your preference?

@stweil
Copy link
Member

stweil commented Nov 7, 2019

I prefer variant 1. The current files src/*/Makefile.am have 1044 lines in total. That's a size which can be handled easily in a single file. Just make sure that things are in some reasonable order, for example sorted alphabetically.

@stweil
Copy link
Member

stweil commented Nov 26, 2019

@amitdo, did you already start working on the non-recursive make, or should I send a pull request?

@amitdo
Copy link
Collaborator Author

amitdo commented Nov 26, 2019

I didn't, so you can send it.

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.

2 participants