build: Completely Redesigned CMake files (#44) #68
Conversation
Primary changes are: - Versioning integrated in cmake build system with library for version usage in end user programs - New minimum CMAke required version is 3.14 - Created install capabilities. Allows uvgRTP to be used in different scenarios for win and lin (mac-os still missing) - Added testing environment with automatically fetched google test and some example test code. - Added packaging capabilities (and some placeholders that have to be changed) - Create Shared library with -DBUILD_SHARED_LIBS=TRUE
|
Force pushed a small change so the default library created will be static. We could talk about creating both or changing de default, but for now I would just keep it that way |
|
What is the state of this, are there any problems I should address ? |
|
It seems no one else has time to address this and if so, I will take look at this next week when I get back from holiday. I will read the code and try running most of the use cases on Windows and Linux. Expect results somewhere between Tuesday and Thursday next week. I can fix minor issues if any are present after accepting this PR. Thank you again for this PR! |
|
Tested compilation on windows and did not encounter any problems. I will do more testing tomorrow and merge this if I don't encounter any major problems. |
|
Couple of points that I would like to ask/mention, but there is no need to change anything: What are those packaging placeholders? I'm assuming they are for creating packages. Should we copy the existing license and readme there? Can uvgRTP_version.hh be just version.hh? I might also change the function names since the namespace already defines uvgrtp. To me it seems a bit excessive to have uvgrtp be twice in file or function path. I can do this after merging. I noticed that there used to be possibility for specifying the Crypto++ library location. I have never tested it (also I don't think uvgRTP needs the static library, just the includes). It would be useful on windows to be able to specify the location of crypto++ since now it has to be done manually in msvc configuration. However, this would be complicated by the fact that uvgRTP uses includes in format crypto/header this requires that the specified folder is parent of crypto. I tested building the shared library of uvgRTP on windows. The building went well, but if I remember correctly, some kind of dll declarations need to be added to uvgRTP for windows in order for the linker to find the functions with DLL. Kvazaar uses __declspec. This can be addressed in future commits since it isn't related to build system. I couldn't figure out how to link Crypto++ to shared uvgrtp on Windows. I know that it is not recommended to use shared version of Crypto++, but I suspect it may be required when using shared version of uvgRTP. I didn't want to spend too much time on this. Overall, a very nice commit. I will merge this tomorrow as is since it is a clear improvement on existing uvgRTP without losing any features. |
There was a problem hiding this comment.
Hi. Thanks again for this great PR. This is my full review and there is only one issue that needs fixing.
Could you fix the versioning in this? Currently, the correct version is 2.0.1. I tried packaging uvgRTP but the name of the package ended up being Project-0.1.1 when it should be something like uvgRTP-2.0.1. This is a bit too large of an issue for me to merge this. Alternatively, this feature can be left out and implemented later.
The problem seems to be that there are many different variables used for versions and some of them are set and some of them are not. There are variables like PROJECT_VERSION_MAJOR and uvgrtp_VERSION_MAJOR, neither of which are set anywhere. I don't fully understand how this works in CMake, but currently the result is not correct.
Could you also change the names of files uvgrtp_version.cc/hh to just version.cc/hh? Also could you change functions get_uvgrtp_version()(major,minor,patch) to just get_version() since the uvgrtp is already present in namespace?
Minor issues that don't need fixing:
- There were several issues with shared lib on Windows, but those can be fixed later if needed. The static lib works as well as before.
- The pedantic parameter causes lots of warnings from Crypto++ headers
Building the tests worked fine.
If you'll fix the base of versioning to work, I will merge this. I can then start tuning details to my liking, document the features and make a release.
Thanks again.
Those will be put into the packages (e.g. the dpkg package).
yeah, I agree
There was a variable LIBRARY_PATHS but in the master branch I was working on, this variable was never used, so I assumed that this is not necessary anymore. I could create a CMake find file so we could use find_package for that, but I would rather do that after the merge
Again, that could simply be done by a "Cmake find" file for Crypto++. I can help you with that. That can even be done so it works on Linux,Windows and Mac-OS
That is strange. I've testet this on windows and linux. The version I've used in that PR is 2.0.0. Are you sure you testet the correct branch ? (unfortunately I have a lot of similar sounding branch names :-/ ) What package under what OS did you test here ? I've testet debian and some kind of windows installer, but there are a lot more that "should" work.
PROJECT_VERSION_MAJOR: Defined HERE uvgrtp_VERSION_MAJOR: Those are also set by cmake HERE |
Ok, I can do that after merging.
Currently, I add Crypto++ manually to Visual Studio (as shown in the instructions :p). Not sure what is the best way to do this.
I simply meant that there were linking errors when I tried to add it. Not specifically related to CMake, but something that relates to conversation on whether shared should be default. It is possible I was linking to a wrong version of Crypto++ or something.
I retested this again, and now it seems to produce the correct result. Maybe this was just an error on my part. I will do a bit more testing next week, but it seems the current situation is acceptable.
I just run cpack on Linux after cmake and see what comes out. Now it produces something with correct names. I have no idea what was the problem last time.
Ok, I was mistaken. It seems the biggest mistake was on my part. I will do a small amount of testing next week if I can figure out what went wrong. There are no changes necessary, but you could change the version number and version file/function names if you would be so kind? |
jrsnen
dismissed
their stale review
It seems it works now that I did the same thing a second time.
|
I merged this. Thanks a lot! I couldn't figure out what I did wrong. If it surfaces, we can address that later. I will now start prepping everything for a release. |
|
Ah ok, thanks... I could have addresses those points of yours.. sorry, was pretty busy the last couple of days :-( |
|
No problem, :) I didn't mean to steal your thunder! The version stuff was so small that I didn't feel it mattered who did it, I just suggested it if you had time. Other small issues can be fixed later if needed. |
Primary changes are: