Refactor CMakeLists and make shared versions work for MSVC

[russelltg]

I pretty much rewrote the CMakeLists.txt file and split it into three files. Mush shorter, much cleaner.

I also made sure we were exporting symbols from haicrypt when building for DLLs, so it compiles and runs now.

Another change is I use find_package in CMake to find the OpenSSL libraries and includes, so if they're in the default location they will be detected automatically. Same for pthread except CMake doesn't supply a FindPthread.cmake file, so it uses some simple find_path and find_library calls.

I updated the README to reflect these changes in the build system.

These changes are tested on windows MSVC 2013 for both static and shared and linux for both static and shared.

Also:

• Up the CMake requirement to 3.0.2
• Add all arbitrary options as offical cmake options

[alexpokotilo]

Great request ! We get the same issues with dynamic build for windows

[ethouris]

That's nice, just please fix the following things:

1. Changing the name from WITH_OPENSSL* to this OPENSSL_LIBRARIES etc. seems to be good (and seems better standard for CMake), but we need the old compatibility to be maintained. This is about the file configure-data.tcl that contains the procedures describing how options for configure are translated into cmake variables (just note: flagval is to simply skip the -L or -I prefix from every list element). And note that if this has to be a convention, then _LIBRARIES is a kinda keyword - so if you have OPENSSL_LIBRARIES, then you should also use PTHREAD_LIBRARIES. Note that the system with separate LIBDIR and LIBRARIES was to have the library directory prefix and the list of library files to be specified separately; make sure that you maintain it so that the user doesn't have to repeat this prefix when specifying the list of OpenSSL library files. You can simply combine them the way they are now, whereas setting the ready path to both files only when this is being autodetected.

2. The connection between debug and logging is the following:
   a. Logging is ALWAYS turned on when compiling in debug mode.
   b. The ENABLE_DEBUG variable enforces setting the debug mode (must be done before checking for debug mode)
   c. SRT_DEBUG_ENABLED is a convenience shortcut for later checking if (SRT_DEBUG_ENABLED) instead of CMAKE_BUILD_TYPE EQUALS "Debug". This actually can be simply changed to ENABLE_DEBUG, just synchronized bidirectionally with CMAKE_BUILD_TYPE.

We need these things preserved.

3. The win_time.cpp is kinda compatibility thing - so the best place to move it is the common directory (simply making it available for haicrypt makes it then derived by srt, howver it logically isn't part of haicrypt, just like other stuff in srt_compat.c file).

4. I don't think renaming ENABLE_DYNAMIC with BUILD_SHARED is good; I'd agree with SHARED, but ENABLED should be preserved, just like for other things.

5. AFAIR the current source file definition is to simply take everything from the source directory, not elist the sources, please maintain it.

6. The header files aren't necessary to be enumerated at all. Private header files are just the matter of the implementation files that use them. Should they be by some reason required on Windows, please extract them also from the current source directory.

7. You are copying all header files into the header installation directory - which is wrong; not all header files are public. There's a HEADERS.maf file, which is a manifest file that defines which files should be copied into the installation directory, please maintain it.

8. Pay attention that OpenSSL is actually a dependency of haicrypt, not the overall project. So, if you split the files into smaller parts, probably the whole OpenSSL detection stuff should go to the haicrypt/CMakeLists.txt. An alternative solution is to make haicrypt compile statically only, and then make the srt library compiled dymamically, with static linkage against haicrypt. Not sure if this is possible or "so easy".

[russelltg]

Thanks for the review!

1. as far as OPENSSL_LIBRARIES, that's not really my choice. I switched it to from manually configured OpenSSL library to use cmake's built in FindOpenSSL file. Although I could just have OPENSSL_LIBRARIES be set to WITH_OPENSSL_LIBRARIES (and maybe prefix the WITH_OPENSSL_LIBDIR if required) before the find_package call.

2. It's possible that it's more pragmatic to just error if CMAKE_BUILD_TYPE isn't set so it's transparent to the caller what mode we're building in. A warning would work too. As far as point a, fixed.

3. Done.

4. I only choose that because that's what's common in CMake (I guess now that I do my research it looks like BUILD_SHARED_LIBS is the most common). Example 1 2 3.

5. Yeah, but that's bad practice in CMake. You can read more about that here.

6. It's nice to enumerate them because then it adds it to the project file in visual studio. If you're against this, I can remove it.

7. Thank you, fixed!

8. Hmm that's an interesting idea. That's definitely possible, but could be difficult depending on how much flexibility you want in building. If you want to be able to build all shared or just one shared, that could get complicated with exports if the client using the DLL wants to be able to call the functions from haicrypt. Also good point on OpenSSL only being a dep of haicrypt, I fixed it (previously it was linking to both).

Also, I realized that the DLLs weren't being put in the same dir as the exectuables (due to the subdirectories). I fixed that as well.

[ethouris]

Ok, so first: I'm really sorry for the confusion. I thought I put already this stuff for extracting the headers to install from HEADERS.maf file - please take the latest version from master on my replica, I've applied these changes. Additionally:

1. What I mean is that the configure script - which is actually a wrapper around cmake - should still work the same way. Simply if you change the names of the variables, please make the changes in configure-data.tcl file so that the new names are reflected.

2. Most users may want to do cmake . once this stuff is in and expect it to compile with default option. We can go with default being Release build.

3. For me the BUILD prefix is kinda ambiguous. I think this can go with just alternative names, should this sound more cmake-users-friendly.

4. I'm aware of these problems, so far we decided that the risk is acceptable. That's why these haicrypt and srtcore directories contain exclusively exactly the things that must be there and nothing else. Optional files that are build option dependent can be stored in separate directories. There could be a different approach made though: instead of creating a CMakeLists.txt file in both these directories, put there some "includable" cmake-syntax files, which will provide the value of haicrypt_SOURCES and haicrypt_HEADERS, as needed (or even simpler: have two listing files that will contain the list of source files and header files. This way you'd avoid using the content-based file list without moving too much outside the main CMakeLists.

5. I thought this might exactly be the reason :) It's not a problem, my concern was rather that it wasn't necessary. But for MSVC I understand it is.

6. There are two approaches possible: either keep the directories contain only the least specification, that is, the file list and possibly some specifics, whereas the complete configuration is in the central cmake, or you can treat them as a kinda separate projects. Actually the latter makes little sense for me because the haicrypt library is not really predicted to be used without SRT, whereas it's a dependency for "SRT Core".

[russelltg]

The header extraction method is great....on unix where tclsh is widely available. On windows this is another buildtime dependency that isn't exactly necessary (if you plan on using cmake instead of the ./configure script). Is it workable for me to either
a. write a parser for your .maf files in pure cmake (might be a tad messy, but totally doable)
b. have the maf files be replaced with cmake (like a headers.cmake file or something). Might not be possible if the Headers.maf files are used outside of the cmake build system.

Looking at the configure-data.tcl file, it seems that you only pass the WITH_* flags to cmake if they aren't specified. Is that right? It seems strange.

Also slightly unrelated question: why do you have the ./configure script when one could just use cmake? Just curious :).

1. Done. I have removed some options that don't really.

2. Sounds good.

3. Alright, it's changed to ENABLE_SHARED.

4. I like that second solution of source list, it's added.

5. I'm confused at the HAVE_CROSSCOMPILER flag. It seems it's not used. The right way to do this in cmake is with a toolchain file.

[ethouris]

Ah, right. I did it from the rush and again forgot about windows. Not sure if there can be created some method to extract the file list from a file of that format using only cmake methods. If so, and if you have a solution for that, it's definitely better.

The reason why I have created this wrapper was that we had an internal request in the company before we have opened SRT. That CMake-based build should not do any autodetection - instead it should rely on the variables to do this. So, I have made a half-way: I've created a user-friendly script that is usable same way as Autotools configure, but it simply translates options to variables. At that time when I did it it was simpler for me to cover all the autodetection stuff (as I also had to widely improve it). Actually now the autodetection things can be also moved to CMake, then the configure script will just remain with translation or kinda "highly unusual" things.

As it comes to HAVE_CROSSCOMPILER -well, one of the things that we had in our files that selected an alternative configuration for some special cases, in this particular case, a special kind of compiler that doesn't support C++11 (so, stransmit can't be compiled) and on ARM. This was also a platform where we know that OpenSSL is available, but any standard method to check it (pkg-config) would fail. So, it was used to assume that OpenSSL is there, just added some LDFLAGS, and done. I hope this thing might be able to be solved some simpler way - by providing some special value for HAVE_OPENSSL passed in the cmake command line. This HAVE_CROSSCOMPILER was for the case when cmake couldn't properly recognize the compiler as crosscompiler - we did extra test and declare that explicitly.

Still haven't reviewed it, this week we have confs in Montreal concerning SRT, will try when I have some spare time, or otherwise only next week. I like much more these changes you do, though.

[russelltg]

As fas as the .maf files go, is it acceptable for them to be replaced by some sort of simpler file (maybe a public.txt file or similar) so it's easier to parse in CMake without tcl? That'd make windows support easier :).

Thank you for all the help getting this in!

[alexpokotilo]

we cannot wait for this to be integrated. Now we have to change CMakeLists.txt manually that leads to manually merge it for each new CMakeLists.txt on trunk.

[ethouris]

Alright, so, @russelltg , please now try to rebaseline this work to the latest version.

[russelltg]

Also if you want I can squash into a single commit :)

[rndi]

Hello @russelltg ,

Could you please rebase your branch with current srt repo master? There are merge conflicts at the moment.

Thanks!

[russelltg]

Merged up :).

[rndi]

Much appreciated @russelltg ! Will wait for @ethouris to have a chance to review as well.

[rndi]

Ok. Merging this, @ethouris will have to catch up at later time.