noloader / cryptopp-cmake

CMake files for Crypto++ project
BSD 3-Clause "New" or "Revised" License
92 stars 67 forks source link

Build fails: unknown target 'shared' #32

Closed yurivict closed 6 years ago

yurivict commented 6 years ago

I tried to incorporate the cmake build into the FreeBSD port, but it fails:

-- Build files have been written to: /usr/ports/security/cryptopp/work/cryptopp-7.0.0
===>  Building for cryptopp-7.0.0
ninja: error: unknown target 'shared'
===> Compilation failed unexpectedly.

I extracted the project into the cmake directory, and copied back into the root of the project:

post-extract:
        @cd ${WRKSRC} && ${MKDIR} cmake && cd cmake && tar xzf ${DISTDIR}/noloader*cmake* --strip 1 && ${CP} * ..
noloader commented 6 years ago

Thanks @yurivict.

Yeah, make static works as expected but make shared flops on FreeBSD 11. I see a lot of if (BUILD_STATIC) and if (BUILD_SHARED) in CMakeLists.txt so I am guessing it should work. I'm not sure how big (or small) the issue is.

I should probably leave it to the CMake experts to untangle. I am liable to do more damage than good.

@abdes, if you have some time can you setup a VM and duplicate the issue?

SylvainCorlay commented 6 years ago

Taking this occasion to thank both @yurivict for trying on freebsd and @noloader for maintaining the cmake build!

I was very disappointed when upstream dropped the cmake support in the last version, and we rely on it for the xeus project and the xeus-cling C++ Jupyter kernel. ++

abdes commented 6 years ago

There is no such target called 'shared' and this is normal.

The target for cryptopp shared lib is cryptopp-shared.

Besides, you don't need to do anything special to get your shared lib. Just follow the instructions as per the README.md in cryptopp-cmake:

git clone https://github.com/weidai11/cryptopp.git
cd cryptopp
git submodule add https://github.com/noloader/cryptopp-cmake.git cmake
git submodule update --remote

# assuming you are still in cryptopp directory
ln cmake/cryptopp-config.cmake .
ln cmake/CMakeLists.txt

# build
mkdir build
cd build
cmake ..
make

By doing so you will get both the archive and the shared library and the test program will be linked against the archive library.

Tested and fully working on the the below environment:

@yurivict please confirm that you are able to proceed successfully with the above instructions so we can close the issue.

noloader commented 6 years ago

Thanks @abdes,

Can we wire things up so that make shared does not produce an error?

It seems like the hard work is done when the shared object is built. Having make shared work as users expect fixes the usability issue.

abdes commented 6 years ago

@noloader the target is called cryptopp-shared and changing its name will break backward compatibility with people who depend on it in their cmake files.

There is really nothing to write to build shared libs. Just type: make. Shared lib is built automatically.

noloader commented 6 years ago

@SylvainCorlay,

I was very disappointed when upstream dropped the cmake support in the last version, and we rely on it for the xeus project and the xeus-cling C++ Jupyter kernel. ++

Yeah, the failure was due to me and my lack of skills.

Once we moved to a separate GitHub we could make lots of folks Collaborators. That could not happen when on Wei's GitHub.

I'm no longer hindering the process. Now my job is to stay out of the way of the experts.

SylvainCorlay commented 6 years ago

What would get them to accept cmake back as part of the main repository?

noloader commented 6 years ago

@abdes,

... the target is called cryptopp-shared and changing its name will break backward compatibility with people who depend on it in their cmake files.

I would be surprised if CMake did not support a similar syntax like Make. Below the recipe is invoked for either shared or dynamic:

.PHONY shared dynamic
shared dynamic: $(LIBOBJS)
    $(CXX) -shared -o libcryptopp.so $(CXXFLAGS) $(LIBOBJS) $(LDFLAGS)

Using alias targets in CMake and How to create alias for targets using CMake? seem to indicate additional target names can be added that refer back to existing targets.

jcfr commented 6 years ago

If ALIAS do not work or it not available for the minimum version of CMake supported by the project, alternative approaches that should add two "alias" targets.

Note that I haven't tested them

add_custom_target(shared  DEPENDS cryptopp-shared)
add_custom_target(dynamic  DEPENDS cryptopp-shared)
if(NOT CMAKE_CONFIGURATION_TYPES)
  add_custom_target(shared 
    COMMAND ${CMAKE_COMMAND} --build ${cryptopp_BINARY_DIR} --target cryptopp-shared --config ${CMAKE_BUILD_TYPE}
    VERBATIM
    )
  add_custom_target(dynamic
    COMMAND ${CMAKE_COMMAND} --build ${cryptopp_BINARY_DIR} --target cryptopp-shared --config ${CMAKE_BUILD_TYPE}
    VERBATIM
    )
endif()
abdes commented 6 years ago

To be clear, the usability issue will be in using that target called ‘shared’. Today the targets are explicit. If you want to link to static you use cryptopp-static and if you want to link to to shared you use cryptopp-shared. I understood that the recommendation is to as much as possible use the static version for better security (DLL injection). The simple command ‘make’ builds both, however if you guys feel that it would be better to have a ‘make shared’ command or ‘make dynamic’ command then probably an alias would do the trick instead of a new custom target. Not sure if lower version cmake supports target alias but it will be easy to check it and make the modification.

It would be nice if @yurivict adds a comment documenting the scenario where it made more sense for him to use ‘make shared’ instead of ‘make’ for future reference.

noloader commented 6 years ago

@abdes,

the usability issue will be

Yes, exactly. It is a usability remediation so that folks who do make shared get what they expect instead of an error. It does not matter of make shared does nothing under the covers. The only thing that matters is the user avoids an error.

mouse07410 commented 6 years ago

One of the goals is to make (no pun intended) the build process using different tools as similar to reach other as possible.

I personally prefer the old-fashioned "make", "make shared", etc. And I find "make cryptopp-shared" rather convoluted.

I might just go in and edit the targets myself... Unless you'd like to do the right thing and add the targets aliases? ;-)

yurivict commented 6 years ago

This problem is gone, thanks!