search

das-labor / neopg

The multiversal cryptoengine!
Other
214 stars 16 forks source link

Improve cmake build system #90

Closed Bjoe closed 5 years ago

Bjoe commented 5 years ago

I have following question:

Bjoe commented 5 years ago

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 commented 5 years ago

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

lambdafu commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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[bot] commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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" :fire:

lambdafu commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.