karastojko / mailio

mailio is a cross platform C++ library for MIME format and SMTP, POP3 and IMAP protocols. It is based on standard C++ 17 and Boost library.
Other
372 stars 98 forks source link

Fix: Mentioned default in the readme differs from the default in CMakeLists.txt #128

Closed LhKipp closed 1 year ago

LhKipp commented 1 year ago

The default for MAILIO_BUILD_SHARED_LIBRARY is ON. However the readme says it is off.

I've only adapted the english readme, as I don't speak chinese. The same misalignment might exist in README_zh.md.

tsurumi-yizhou commented 1 year ago

I would translate the doc. However, I guess the default value may be ON?

karastojko commented 1 year ago

The default value is ON, but it can be set to OFF from the command line or by editing the Cmake file. Please add the change in the Chinese version, then I could merge the PR.

LhKipp commented 1 year ago

May I allow myself to expand the scope of this MR by the last 2 commits pushed? ( 15df8954617af23d0c44aa46754431d9f59adf13 and 5ab82273fa176564406c46c512d25c1cf144ec13).

CMake has a default option to decide whether a static or shared library shall be build. In the first commit I remove the mailio specific option and use the standard one. I've tested, that the new cmake file works as expected by executing the following commands:

mkdir build && cd build
cmake .. -DBUILD_SHARED_LIBS=ON && make
# A shared lib is build

cd .. && rm -r build
mkdir build && cd build
cmake .. -DBUILD_SHARED_LIBS=OFF && make
# A static lib is build

cd .. && rm -r build
mkdir build && cd build
cmake .. && make
# A shared lib is build

Additionally I've removed unit_test_framework from the link interface of the mailio library and added it to the tests. Currently users need to have unit_test_framework installed when linking with mailio (as mailio links it). Now users don't.

If this MR is accepted as is, @tsurumi-yizhou could you please provide me a patch for the chinese readme (or could you follow up with another MR)? Thanks 👍🏻

tsurumi-yizhou commented 1 year ago

May I allow myself to expand the scope of this MR by the last 2 commits pushed? ( 15df895 and 5ab8227).

CMake has a default option to decide whether a static or shared library shall be build. In the first commit I remove the mailio specific option and use the standard one. I've tested, that the new cmake file works as expected by executing the following commands:

mkdir build && cd build
cmake .. -DBUILD_SHARED_LIBS=ON && make
# A shared lib is build

cd .. && rm -r build
mkdir build && cd build
cmake .. -DBUILD_SHARED_LIBS=OFF && make
# A static lib is build

cd .. && rm -r build
mkdir build && cd build
cmake .. && make
# A shared lib is build

Additionally I've removed unit_test_framework from the link interface of the mailio library and added it to the tests. Currently users need to have unit_test_framework installed when linking with mailio (as mailio links it). Now users don't.

If this MR is accepted as is, @tsurumi-yizhou could you please provide me a patch for the chinese readme (or could you follow up with another MR)? Thanks 👍🏻

The patch has pr to your fork branch.

LhKipp commented 1 year ago

@tsurumi-yizhou Thanks for providing a patch for the chinese readme. I've merged it into this MR and renamed the option name to reflect the option name change from this PR (MAILIO_BUILD_SHARED_LIBRARY->BUILD_SHARED_LIBS).

LhKipp commented 1 year ago

@karastojko Can you please take a look at this PR (relevant comment explaining what I've done, since you looked at it the first time: https://github.com/karastojko/mailio/pull/128#issuecomment-1590630666. Additionally I've changed the chinese README as requested).

karastojko commented 1 year ago

I will. Sorry for the delay, I was trying to finish the Vcpkg setup PR. It has also introduced changes to the CMakeLists.txt, thus I've tried to keep all the other changes to minimum.

LhKipp commented 1 year ago

Ah okay. I thought this PR has become forgotten.

Feel free to finish the work on the vcpkg-pr first. I can rebase this branch.

karastojko commented 1 year ago

@LhKipp I have added you to the contributor list in the Readme file. @tsurumi-yizhou Can you please add the same to the Chinese version from this commit?