Skip to content

Set Corrade::rc as GLOBAL when cross-compiling#101

Closed
Amphaal wants to merge 1 commit into
mosra:masterfrom
Amphaal:master
Closed

Set Corrade::rc as GLOBAL when cross-compiling#101
Amphaal wants to merge 1 commit into
mosra:masterfrom
Amphaal:master

Conversation

@Amphaal

@Amphaal Amphaal commented Oct 26, 2020

Copy link
Copy Markdown
Contributor

corrade_add_resource seems unable to find corrade-rc unless I set the imported target as GLOBAL.

image

This change is quite harmless IMO, but maybe we should add a comment to justify it ?

Cheers !

@codecov

codecov Bot commented Oct 26, 2020

Copy link
Copy Markdown

Codecov Report

Merging #101 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #101   +/-   ##
=======================================
  Coverage   97.56%   97.56%           
=======================================
  Files          98       98           
  Lines        7185     7185           
=======================================
  Hits         7010     7010           
  Misses        175      175           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d232ffb...921d06d. Read the comment docs.

@mosra mosra added this to the 2020.0b milestone Oct 30, 2020
@mosra

mosra commented Oct 30, 2020

Copy link
Copy Markdown
Owner

This happens when crosscompiling and having Corrade as a CMake subproject, right? I'm not quite sure why would that be needed (no other target needs this GLOBAL export), will investigate further and merge.

@mosra

mosra commented Nov 14, 2020

Copy link
Copy Markdown
Owner

Uh oh, how has it been fifteen days already 😅

So, I investigated with a subproject setup targeting Emscripten and realized that if I do this:

add_subdirectory(corrade)
find_package(Corrade REQUIRED Utility)
corrade_add_resource(BLAH resources.conf)
add_library(main ${BLAH})

which is the documented way, it works. While if I do this:

add_subdirectory(corrade)
find_package(Corrade REQUIRED) # or no find_package at all
corrade_add_resource(BLAH resources.conf)
add_library(main ${BLAH})

it doesn't. I gave this a bit of thought, and among all alternatives, setting the IMPORTED target as GLOBAL was the best tradeoff after all (commited as 2cc7728) -- I could require users to find_package(), but documenting (and explaining) why is that necessary even though everything else works is more effort than having things "just work" :) Besides that, in 6a34c96 I equipped all corrade_add_*() CMake functions with checks for presence of their target dependencies, so next time a similar hiccup happens, you know early enough and not late during ninja run.

@mosra mosra closed this Nov 14, 2020
@mosra

mosra commented Nov 14, 2020

Copy link
Copy Markdown
Owner

Hmm, I just realized that when find_package() isn't used, the CMake CORRADE_TARGET_* variables don't get defined. Which means find_package() is still quite important and I'm thinking of just requiring it in all cases. Ideas? :)

@Amphaal

Amphaal commented Nov 14, 2020

Copy link
Copy Markdown
Contributor Author

Hmm, I just realized that when find_package() isn't used, the CMake CORRADE_TARGET_* variables don't get defined. Which means find_package() is still quite important and I'm thinking of just requiring it in all cases. Ideas? :)

https://stackoverflow.com/a/6891527/3021058, PARENT_SCOPE on every relevant variables ?

@mosra

mosra commented Nov 15, 2020

Copy link
Copy Markdown
Owner

That won't help if you have the submodule in 3rdparty/corrade and all your sources in src, for example. These would need to go into the cache, but then I'm not sure about the interactions with magnum, where find_package(Corrade) gets called, retrieving those variables again.

I'd say just use find_package() always, to avoid this and potential other yet-undiscovered issues ;)

@Amphaal

Amphaal commented Nov 15, 2020

Copy link
Copy Markdown
Contributor Author

To circumvent thoses kind of issues when a dependency is shared amongst multiples subprojects, I always use some kind of a guard with something scopeless like :

if(NOT TARGET spdlog)
    find_package(spdlog REQUIRED)
endif()

In this exemple, if spdlog target is not included as a subproject or already found somewhere else, I can automatically attempt to fetch it as IMPORTED target. Indeed, that's not quite an answer for variables which are not really bound to a TARGET, but a single cached variable like CORRADE_INCLUDED_FROM_SOURCE only set to ON in the main CMakeLists.txt of corrade can do the trick.

IMO something here is missing in CMake to help detect multiple project() usage https://gitlab.kitware.com/cmake/cmake/-/issues/17317

@mosra

mosra commented Nov 18, 2020

Copy link
Copy Markdown
Owner
if(NOT TARGET spdlog)
    find_package(spdlog REQUIRED)
endif()

I don't think the users should need to care about this. I commonly call find_package() multiple times with different components in different subdirectories and expect that to work, and I also expect that to continue to work when the package is suddenly not fetched from the system but add_subdirectory()'d.

IMO something here is missing in CMake to help detect multiple project() usage https://gitlab.kitware.com/cmake/cmake/-/issues/17317

In my workflow at least, that's not a problem either, because I just unconditionally call find_package() always, and I expected find_package() to do the hard work of discovering whether it's there as a subproject, as an imported target or as a bunch of cmake-less libraries and includes on the filesystem, and provide me with a single interface covering all those variants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

2 participants