Improve cmake build system

[Bjoe]

• Cleanup cmake build system to best practices cmake practices #83 .
• Easily import neopg project into any IDE/development environment
• Add proper install target build or install broken on Ubuntu 18.04 #81 . Install headers and libs in a selected directory (via CMAKE_INSTALL_PREFIX)
• Create a cmake config modul to easily import neopg to other cmake project via find_package(neopg)

I have following question:

• It is correct that the "legacy" part should not exported/installed? See difference between 0f953d9 and 816152a
• At the moment, libneopg-tool.a with the include headers from neopg-tool are exported/installed? Should I also remove this from the install/export target?

[sphinxc0re]

First of all: Thank you for contributing to this project. We very much appreciate new members of the community. Secondly, this PR seems way too big. Could you possibly split this up into multiple PRs?

The code looks pretty good to me, but I still have some regards.

[Bjoe]

Secondly, this PR seems way too big. Could you possibly split this up into multiple PRs?

Unfortunately, yes the PR is big. The effort will be increase if I must split this PR. Maybe I found a solution of that. Let me think about that.....

[Bjoe]

Please wait with the merge. I need to fix my review findings. Give me a day :-)

[lambdafu]

Thanks for working on this! To answer your questions: Nothing in legacy should every see the light of day, so not exporting it is correct. And libneopg-tool exists only to allow for creating a separate testing binary that does unit testing with code from the main program. So that should be linked statically and not exported either. There was a ticket for revisiting cmake (#83). The goal is to be as "normal" as possible. I think the main painpoint is the step that collects all header files, but having the header files next to the source files and tests is important to me for navigating in the text editor, but maybe c++ developers deal with this differently? Let me know if I am missing something!

[Bjoe]

And libneopg-tool exists only to allow for creating a separate testing binary that does unit testing with code from the main program.

Ok, this means it should also not "exported". I will remove this from the cmake install target.

having the header files next to the source files and tests is important to me for navigating in the text editor, but maybe c++ developers deal with this differently?

No, that's ok in my opinion. It's like code formatting I think. Some would say, it is better to have source and headers separated and some are not. I am not very religious about that :-).
But it should be "consistent", for example if the header file signature_expiration_time_subpacket.h is in the directory neopg/openpgp/signature/subpacket/ the include path should be then

#include <neopg/openpgp/signature/subpacket/signature_expiration_time_subpacket.h>

Otherwise it is not possible to "import" the project into the "development environment" after git clone. Also it is a bit confusing when a compile error occures in a header file and the header file is not in the same folder as the error indicates.
In my opinion, it should be "easy to use" or?
Now, it is maybe a good idea to rename the header file to a "shorter" name, for example

#include <neopg/openpgp/signature/subpacket/expiration_time.h>

[Bjoe]

Unfortunately to get travis running again takes more time that I expected. I see also some compile warnings and errors.
Give me some more days, I will fix this before we merge this PR.

[codecov]

Codecov Report

Merging #90 into master will increase coverage by 0.55%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
+ Coverage   20.53%   21.09%   +0.55%     
==========================================
  Files         294      368      +74     
  Lines       32779    33033     +254     
==========================================
+ Hits         6732     6967     +235     
- Misses      26047    26066      +19

[lambdafu]

Again, thanks for working on this. Everything seems fine, but I am not sure I'm 100% convinced that it is better to use subdirectories for the header files. Before this patch we're basically doing the same as Botan is doing, and it seems to work for them. The benefit of dropping the directory component: You can reorganize the files without changing the API. And the user has a bit less to copy/keep in their head. Of course, the downside is you won't notice if you use the same name twice, and the whole header copying during the build. I'm not super happy with either. OTOH, if we expect users to include only neopg/neopg.h anyway, most of this doesn't matter. Another point: If C++ modules ever arrive we might as well do now what will work best then?

[lambdafu]

With regards to the name: See, the pressure to shorten the name conflicts with a desire to be consistent with the naming of the header files matching the class name matching the OpenPGP packet name. "A foolish consistency is the hobgoblin of little minds" 🔥

[lambdafu]

I guess we can redo the header file location stuff at a later time. Don't want to drag you through another round of editing and testing!

[Bjoe]

Hi. "Oh" you merge already my PR :-), thanks but there was some missing open task that I fixed now: Don't export neopg-tool libs and headers.
Should I create a new pull request for these fix?
Also with your concern about the "header" location. I was not expecting that botan also use this "header approach". I would like to investigate/find some solution ... but it will take some weeks because I have something to do for the upcoming MeetingC++ conference in Berlin. (Is anyone of you there?)
Maybe we discuss about the "header location" on an issue in Github?

[lambdafu]

Sure, just make a new pull request. As @sphinxc0re said, it would have been preferable to separate it a bit anyway :). And wrt to the header naming, we can discuss in a ticket, sure. No pressure. Thanks again for the great work so far!

[Bjoe]

Ok I create quickly the PRs see #92 and #93

As @sphinxc0re said, it would have been preferable to separate it a bit anyway :). And wrt to the header naming, we can discuss in a ticket, sure.

When the conference in Berlin is over, I will be back here.