Closed kenkit closed 4 years ago
Everything is fine on appveyor with https://ci.appveyor.com/project/kenkit/bit7z
But I think you should enable building of pull requests on appveyor so that pull requests will have to be tested
We have to choose cmake generator, either 32bit or 64bit. This also includes the /MT flag. I guess we will have to set two environment variables that will be passed to cmake script. One will include the cmake generator to be used the ether will check whether the MT flag should be updated. I'll leave that for later.
I'm not sure if auto-detect should be enabled by default in cmake.
I can enable it with target_compile_definitions(bit7z PRIVATE -DBIT7Z_AUTO_FORMAT=1)
If you want it to be on in the installed lib files by default.
Hi!
First of all, thank you for your pull request!
I've been planning to migrate the project to CMake for a long time.
I even have a basic CMakeList.txt
file but I never refined it to focus on the functionalities of the library (hence the exclusion line in the .gitignore
file).
Motivation and Context
Cmake is a much more flexible and crossplatform build tool It should be possible for users to use other complilers like mingw(Not tested) I am a user and also partly a maintainer of https://github.com/getnamo/7zip-cpp which offers features nearly similar to bit7z, but the projects developement has stalled.
I must be honest: I don't like CMake (for various reasons which are not important here).
At the moment, bit7z compiles only under MSVC so other compilers cannot be used (a cross platform version is in the working, with some good results).
Anyway, CMake is without doubt one of the most used build tool for C++ projects and it is supported by many IDEs (unlike qmake
and msbuild
), so there are good reasons to adopt it for bit7z, considering also the future support to other compilers/platforms.
Add ctest framework after a test suite is added using google tests
Yeah, I actually have a pretty complete test suite (using Catch2) which however I still don't publish since it contains a lot of "ugly" and unmaintainable code. I will try to heavily refactor and improve it in order to have it openly available (and for using it in the CI).
Everything is fine on appveyor with https://ci.appveyor.com/project/kenkit/bit7z
Actually, at least for the moment I would like to retain at least the .vcxproj
project file (along with CMakeLists.txt) since it simplifies the build configuration of AppVeyor CI.
So I'm not really interested in changing the appveyor.yml
, anyway I'll review your changes in detail.
We have to choose cmake generator, either 32bit or 64bit. This also includes the /MT flag. I guess we will have to set two environment variables that will be passed to cmake script. One will include the cmake generator to be used the ether will check whether the MT flag should be updated. I'll leave that for later.
This is exactly one of the reasons of why I don't like CMake: too many commands and arguments for simple tasks. Anyway, I'll review this.
I'm not sure if auto-detect should be enabled by default in cmake. I can enable it with
target_compile_definitions(bit7z PRIVATE -DBIT7Z_AUTO_FORMAT=1)
If you want it to be on in the installed lib files by default.
No, it should not be enabled by default. It is a feature that takes a lot of space (due to the maps of file signatures/extensions used to detect the formats) and many users of bit7z probably will never use it since they know the format they are using. So I decided to make it disabled by default.
Anyway, I'll review the commits asap.
This is exactly one of the reasons of why I don't like CMake: too many commands and arguments for simple tasks.
Currently you can use any toolcachain you want x86/x64, the only fix will have to be in appveyor and the /MT flag. So all the cmakelists needs is addition of MT flag, Most of the other changes will only be in the appveyor.yml
No, it should not be enabled by default. It is a feature that takes a lot of space (due to the maps of file signatures/extensions used to detect the formats) and many users of bit7z probably will never use it since they know the format they are using. So I decided to make it disabled by default.
Fixed the auto-detect format to be disabled by default.
The correction in the README.md file was correct, I totally forgot to add the using namespace bit7z
line in the examples, thank you!
In addition, I decided to substitute the int main()
function with a try-catch
block, which I think to be more useful for new users of bit7z.
Nice, I was wondering how to catch exceptions, but I figured manager after looking into the source.
Hi! I've made some changes to the CMakeLists.txt file you proposed.
bit7z${ARCH_POSTFIX}${CMAKE_DEBUG_POSTFIX}
(e.g. bit7z64_d
on x64 debug builds) as in the current project files.${PROJECT_SOURCE_DIR}/bin/${ARCH_DIR}/
(e.g. ./bin/x64/
) as it is in the current project files./Os
optimization for releases builds and the /W4
for the warning level.install
target: on Windows it seems to be buggy (at least to me it ignores the prefix, for example...) and the appveyor builds can be configured also without using it.Probably with the introduction of the support to Linux/p7zip (as well as MinGW) other changes will be required, but I would say to leave them for the future (since we are merging to v3.1).
Since this will be included in the v3.1 minor release, I would like to maintain the already existing project files (i.e. bit7z.pro
and bit7z.vcxproj
).
Obviously maintaining three different project files is not ideal in the long term and probably at least one of the two will be deleted (probably bit7z.pro
).
Please let me know if you have any doubt or suggestion!
That's some nice work.
Regarding the configuration of AppVeyor, although I think you can use CMake for that, I think it is better to take a more conservative approach and keep the current settings, at least for the moment (also considering that the Visual Studio project file will remain in this version of bit7z). Obviously in the future it will be necessary to arrive at only one project file and then modify the AppVeyor configuration accordingly.
Thanks for merging, I'm hardly free this days. Regarding a fix for cmake we could do something like this depending on the variables being set
set_property(TARGET foo PROPERTY
MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
It's much easier maybe use if(VARIABLE_MSCVRT) ..else... EDIT:The above will work for cmake 3.15+
Regarding a fix for cmake we could do something like this depending on the variables being set
set_property(TARGET foo PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
It's much easier maybe use if(VARIABLE_MSCVRT) ..else... EDIT:The above will work for cmake 3.15+
I too considered the possibility of such fix, but unfortunately — as you noticed — it is only available in the latest version of CMake.
Hence, I decided (commit b927437efe0030c47fbac9ea9f0efd1a73d8271c) to take the alternative approach explained in https://stackoverflow.com/a/56520824, and enable it by using the flag -DSTATIC_RUNTIME=True
.
Hi @kenkit! I'm contacting you since you contributed to this project with this PR. After many evaluations, I decided to change the license of bit7z from the GPLv2 to the Mozilla Public License (MPLv2). This change should allow the library's usage to grow further, avoiding the viral aspects of the GPLv2 license. Since this is an open-source project, I need the approval of all the contributors before proceeding with the re-licensing. So, if you are OK with the license change, could you kindly reply to the re-licensing issue page (https://github.com/rikyoz/bit7z/issues/86) with the message "I hereby agree to publish my contributions to bit7z under the MPLv2 license"? If, instead, you're against it, could you let me know the reasons? Thank you in advance!
Enable build with cmake also fixups to appveyor.yml
Description
This change will allow anyone to generate project files for any project
Motivation and Context
Cmake is a much more flexible and crossplatform build tool It should be possible for users to use other complilers like mingw(Not tested) I am a user and also partly a maintainer of https://github.com/getnamo/7zip-cpp which offers features nearly similar to bit7z, but the projects developement has stalled.
How Has This Been Tested?
Types of changes
Checklist:
Add ctest framework after a test suite is added using google tests