Update project layout - #267
Conversation
Proxy headers have been placed into the project root to emit deprecation warnings.
The new code does not rely on globally defined include directories anymore. Additionally a lot of conditional code and has been removed which improves readability a lot.
This makes sure that the test does not break due to other reasons.
|
Thanks for your work on improving the build system! I'll look into this in more details, but for now just a quick comment that Appveyor build is failing: https://ci.appveyor.com/project/vitaut/cppformat/build/1.0.1521 |
|
I know, but fixing the windows build is non trivial on a Linux machine without any usable hints from the build log. Anyway, I think I figured it out now. ;) |
Thanks! I often break Windows build myself, and then have to fix it based on CI logs =). I've started looking at the commits and everything looks fine so far (nice job), just a few minor comments. |
|
Thank you for the detailed review and the great support! It is very much appreciated! I will address your comments as soon as possible. |
|
I've finally finished reviewing the PR (that was a lot of changes =)) and I should say your CMake-fu is much better than mine. Hope the amount of comments is not intimidating, these are mostly minor things. In general I think this is a great improvement and thanks again for working on it. |
Additional notes on how to improve the current state have been added.
Based on that information less intrusive option defaults are choosen. Additionally, packaging support is omitted.
Conflicts: posix.h
|
Not at all, you are welcome! I think I addressed most of your comments now. The only thing I have seen just now is that Travis finds some warnings which are not reported by my local compiler. I need to have a look at those next. |
|
Merged, thanks! |
|
Nice. Thank you! |
|
Is there a reason that the cppformat.cc is installed? Usually, only header files are installed. Also, should the .cmake files be included in the packaging for Fedora? If so where? One last thing is that in the .zip file, the .cmake files are in the the build directory, but that conflicts with the build instructions. |
|
The Yes, including the generated cmake files into the packaging would be important. Otherwise the find "magic" does not work. Note that you have to install the cmake files which are installed when you call According to the cmake documentation the following paths are searched by cmake on unix: I currently install to Could you elaborate on your last point? Exactly what .zip file are you talking about? |
|
Ok, header-only mode sounds like an interesting feature, but it's akin to static linking, which is prohibited in Fedora (and many distributions), so I won't package the files to support it. I'll include the .cmake files, but in Fedora, the path is /usr/share//cmake/, so can that be made configurable? I was referring to the .zip file for the release. In the .zip, there's a directory called build that has the .cmake files in it and so when you try to follow the build instructions, the mkdir fails. |
|
Fair enough. Making it configurable is no big deal, I'll follow up with a PR. I have no idea why the build directory with the cmake files is in the release zip file but I am sure @vitaut can shed some light here. |
|
The build directory got into 2.x by mistake when I merged the new CMake config. Fixed in 8019387. |
3 participants
This PR implements the change which has been discussed in #264.
It basically moves the source files into a cppformat subdirectory and performs a major update on the cmake scripts. This was necessary in order to properly test if the includes and defines are properly propagated with the library targets. I think that I could considerably simplify the build system which should also benefit the future development.
Please have a look at my changes and tell me what you think. :)