Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Easy to forget checking FIPS_IMPORT in a "dual use" project (import vs top-level) #230

Open
kaffeewolf opened this issue Jun 26, 2019 · 2 comments

Comments

@kaffeewolf
Copy link
Contributor

When an imported project already contains a .fips-imports.cmake it will cause the libraries referenced by that project to be added twice, causing cmake to fail

Proof of concept:
fips-import.zip

  • fips-import-broken imports fips-import which imports fips-glew.
  • running fips gen in fips-import-broken with a clean fips-import project succeeds
  • running fips gen in the fips-import and then again in fips-import-broken fails with
CMake Error at D:/devel/fipstest/fips/cmake/fips.cmake:299 (add_library):
  add_library cannot create target "glew" because another target with the
  same name already exists.  The existing target is a static library created
  in source directory "D:/devel/fipstest/fips-glew".  See documentation for
  policy CMP0002 for more details.
Call Stack (most recent call first):
  D:/devel/fipstest/fips-glew/CMakeLists.txt:17 (fips_end_lib)

Note: the urls in the fips-imports projects are obviously wrong, but it doesnt matter as they are already present in the folder structure.

CMake version 3.14.1, python 3.6 and 3.7

@floooh
Copy link
Owner

floooh commented Jun 26, 2019

Ok, I can reproduce by first running fips gen in fips-import, and then in fips-import-broken.

I think the bug is that the .fips-imports.cmake of dependencies seems to be included in top level projects, that shouldn't happen. Only the .fips-imports.cmake file in the top-level project itself is relevant. I'm surprised this problem didn't show up yet :)

@floooh
Copy link
Owner

floooh commented Jun 26, 2019

Ah alright, I found the underlying problem, but I agree that the fix isn't a very good solution...

Basically the top-level CMakeLists.txt file in fips-import looks like this:

cmake_minimum_required(VERSION 2.8)
get_filename_component(FIPS_ROOT_DIR "../fips" ABSOLUTE)
include("${FIPS_ROOT_DIR}/cmake/fips.cmake")

fips_setup()
fips_project(fips-import)
fips_begin_lib(dummy)
fips_src(dummy)
fips_deps(glew)
fips_end_lib()
fips_finish()

fips_setup() and fips_finish() are always called, no matter if this is used as a top-level project, or used as an imported project.

For such "dual-use" projects, the top-part up to (and including) fips_project() and the final fips_finish() must be excluded if the variable FIPS_IMPORT is defined.

For instance see the toplevel CMakeLists.txt file in fips-glfw:

https://github.com/floooh/fips-glfw/blob/master/CMakeLists.txt

So the above fips-import CMakeLists.txt file would need to look like this:

if (NOT FIPS_IMPORT)
    cmake_minimum_required(VERSION 2.8)
    get_filename_component(FIPS_ROOT_DIR "../fips" ABSOLUTE)
    include("${FIPS_ROOT_DIR}/cmake/fips.cmake")
    fips_setup()
    fips_project(fips-import)
endif()
fips_begin_lib(dummy)
fips_src(dummy)
fips_deps(glew)
fips_end_lib()
if (NOT FIPS_IMPORT)
    fips_finish()
endif()

I could hide part of this check inside fips_setup()/fips_finish(), but the problem remains with the cmake_minim_required() call and including fips itself... but I will keep this ticket open as a remainder that there should be a better solution...

@floooh floooh changed the title Presence of a .fips-imports.cmake in an imported project causes douple imports of those libs Easy to forget checking FIPS_IMPORT in a "dual use" project (import vs top-level) Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants