[cmake] Add alias target expat::expat

[Vollstrecker]

As the find module and the config create the expat::expat alias for expat, the build-process should do this also to make it easier to include this in a chainbuild

[hartwork]

@Vollstrecker I'm not sure I understand the issue. Could you elaborate what problem this is solving? At the risk of missing something obvious: Is anything keeping a projecting including Expat on CMake level keeping from adding a line like that themselves and how would it help them? Thanks in advance!

[Vollstrecker]

The examples show how to search in config mode and then using target_link_libraries(...expat::expat) as this target is created by the config.

If there no local installation is found I could use fetchcontent (git submodule or a copy of the code would also work) and just let expat be built and installed in one run of cmake instead of first config/build/install expat and then again config/build/install my project.

If done that way, I would have to use target_link_libraries(... expat), as only that target is created atm.

[hartwork]

@Vollstrecker I've been experimenting with FetchContent a bit myself again now and found this…

cmake_minimum_required(VERSION 3.11)

include(FetchContent)

project(hello VERSION 1.0.0)

FetchContent_Declare(
    expat
    GIT_REPOSITORY https://github.com/libexpat/libexpat/
    GIT_TAG        88b3ed553d8ad335559254863a33360d55b9f1d6  # i.e. R_2_6_3
)

FetchContent_MakeAvailable(expat)

add_subdirectory("${expat_SOURCE_DIR}/expat")

add_executable(hello
    hello.c
)

target_link_libraries(hello PUBLIC expat)

…to work as expected and…

target_link_libraries(hello PUBLIC expat::expat)

…to only work after something like…

if (NOT TARGET expat::expat)
    add_library(expat::expat ALIAS expat)
endif()

…before that, on top.

My personal conclusion from that is that:

• Use via FetchContent could be documented as "case c" in the readme, and there is a draft pull request README.md: Document use of Expat via CMake >=3.18 with FetchContent and SOURCE_SUBDIR #905 for it now, where I welcome review, if you like.
• I worry that if config mode does register expat::expat for us — which it does, via add_library(expat::expat SHARED IMPORTED), I checked — unconditionally aliasing it ourselves means that the two calls could be competing and I worry that may backfire either now or with future versions of CMake.
• I personally do not yet see value in adding alias expat::expat for the FetchContent case yet, because…
  • it's easy and low-risk to add a conditional alias outside of libexpat, I verified it's easy (see above) and works,
  • the three ways to use Expat via CMake seem different enough and need a conscious choice, that I'm not sure unification is worth anyone's time before combining multiple of those approaches at the same time is reported as common.

I think I'm still missing part of the picture of your use case? I'm happy to learn what this picture of mine is missing. Thank you!

[Vollstrecker]

* Use via `FetchContent` could be documented as "case c" in the readme, and there is a draft pull request [`README.md`: Document use of Expat via CMake >=3.11 with `FetchContent` #905](https://github.com/libexpat/libexpat/pull/905) for it now, where I welcome review, if you like.

I'm just on the run now, so I will come to back this next wed.

* I worry that if config mode does register `expat::expat` for us — which it does, via `add_library(expat::expat SHARED IMPORTED)`, I checked — unconditionally aliasing it ourselves means that the two calls could be competing and I worry that may backfire either now or with future versions of CMake.

Ecactly, but the config is not executed if it wasn't installed (or if someone just executes fetchcontent/git submodule/whatever) to test against head or just use a newer version. In this case the source itself is included and the alias is never created, that's why it needs to be created here. Having the Config and the CMakeLists.txt executed in one run would be a bad mistake by a user.

* I personally do not yet see value in adding alias `expat::expat` for the `FetchContent` case yet, because…
  
  * it's easy and low-risk to add a conditional alias outside of libexpat, I verified it's easy (see above) and works,
  * the three ways to use Expat via CMake seem different enough and need a conscious choice, that I'm not sure unification is worth anyone's time before combining multiple of those approaches at the same time is reported as common.

I didn't read the doc yet, but noone should pull in a lib two different ways in one run. With the new function in fetchcontent it's possible to let the system know about the repo, so find_package will either find it with the config or download it via fetchcontent. It's always a one or the other situation and then one line here is much better than 3 lines in every program that wants to use to lib.

I think I'm still missing part of the picture of your use case? I'm happy to learn what this picture of mine is missing. Thank you!

You just think about what happens if users use your stuff the wrong way. In regards of the code itself (memleaks etc.) it's the best approach you can do. In this case you would support users that will have other problems with their approach, but otoh. any dev that uses it right need to do extra steps to get a consistent usage of the lib.

No dev should need to care about where the lib comes from.

[hartwork]

• I worry that if config mode does register expat::expat for us — which it does, via add_library(expat::expat SHARED IMPORTED), I checked — unconditionally aliasing it ourselves means that the two calls could be competing and I worry that may backfire either now or with future versions of CMake.

Ecactly, but the config is not executed if it wasn't installed (or if someone just executes fetchcontent/git submodule/whatever) to test against head or just use a newer version. In this case the source itself is included and the alias is never created, that's why it needs to be created here. Having the Config and the CMakeLists.txt executed in one run would be a bad mistake by a user.

@Vollstrecker understood, I figured after my previous reply that those two add_library calls would never be run at the same time, and hence not compete.

I didn't read the doc yet, but noone should pull in a lib two different ways in one run. With the new function in fetchcontent it's possible to let the system know about the repo, so find_package will either find it with the config or download it via fetchcontent. It's always a one or the other situation and then one line here is much better than 3 lines in every program that wants to use to lib.

So we're only talking about FetchContent with

• a) explicit use of FIND_PACKAGE_ARGS with CMake >=3.24
• b) explicit use of OVERRIDE_FIND_PACKAGE with CMake >=3.24
• c) a non-default value of always for ${FETCHCONTENT_TRY_FIND_PACKAGE_MODE} with CMake >=3.24

…then for FetchContent to even do anything find_package — correct? Which of these is your case? Is yet another case missing?

For unpatched Git master and case (a) this seems to be a well-working hybrid:

cmake_minimum_required(VERSION 3.24)

include(FetchContent)

project(hello VERSION 1.0.0)

FetchContent_Declare(
    expat
    GIT_REPOSITORY https://github.com/libexpat/libexpat/
    GIT_TAG        88b3ed553d8ad335559254863a33360d55b9f1d6  # i.e. R_2_6_3
    FIND_PACKAGE_ARGS
)

FetchContent_MakeAvailable(expat)

if(NOT expat_FOUND)  # with regard to find_package
    add_subdirectory("${expat_SOURCE_DIR}/expat")
    add_library(expat::expat ALIAS expat)
endif()

add_executable(hello
    hello.c
)

target_link_libraries(hello PUBLIC expat::expat)

Given the apparent need for conditional add_subdirectory, the add_library alias call does not seem like much of a deal to me personally.

I should note that adding add_library(expat::expat ALIAS expat) on Expat's side now will break any build systems that
already define that alias without an if(NOT TARGET expat::expat) [..] endif() guard around on their side (including my own example above), with this error (with e.g. CMake 3.29.3):

CMake Error at CMakeLists.txt:19 (add_library):
  add_library cannot create ALIAS target "expat::expat" because another
  target with the same name already exists.

For option case (b) I only find use in OVERRIDE_FIND_PACKAGE for when you do not control the call to find_package(expat [..]).
Case (c) seems to fall back to (a).

I'm just on the run now, so I will come to back this next wed.

Thanks for the heads up, see you!

[Vollstrecker]

We're only talking about FetchContent with

* a) explicit use of `FIND_PACKAGE_ARGS` with CMake >=3.24

* b) explicit use of `OVERRIDE_FIND_PACKAGE` with CMake >=3.24

* c) a non-default value of `always` for `${FETCHCONTENT_TRY_FIND_PACKAGE_MODE}` with CMake >=3.24

…then for FetchContent to even do anything find_package — correct? Which of these is your case? Is yet another case missing?

My Case ist find_package and if(NOT EXPAT_FOUND) fetchcontent, which basically happens internally in all your cases.

For unpatched Git master and case (a) this seems to be a well-working hybrid:

cmake_minimum_required(VERSION 3.24)

include(FetchContent)

project(hello VERSION 1.0.0)

FetchContent_Declare(
    expat
    GIT_REPOSITORY https://github.com/libexpat/libexpat/
    GIT_TAG        88b3ed553d8ad335559254863a33360d55b9f1d6  # i.e. R_2_6_3
    FIND_PACKAGE_ARGS
)

FetchContent_MakeAvailable(expat)

if(NOT expat_FOUND)  # with regard to find_package
    add_subdirectory("${expat_SOURCE_DIR}/expat")
    add_library(expat::expat ALIAS expat)
endif()

add_executable(hello
    hello.c
)

target_link_libraries(hello PUBLIC expat::expat)

Given the apparent need for conditional add_subdirectory, the add_library alias call does not seem like much of a deal to me personally.

As fetchcontent_makeavailable does the add_subdirectory after find_package did the if, the only thing left ist the alias, that's the reason for this change.

I should note that adding add_library(expat::expat ALIAS expat) on Expat's side now will break any build systems that already define that alias without an if(NOT TARGET expat::expat) [..] endif() guard around on their side (including my own example above), with this error (with e.g. CMake 3.29.3):

Sure, but this means someone ist pulling in head unconditionally and a) hopes for the best which never is a good Idea or b) they want to See If Something Breaks to have time to react before the Release.

As you already noticed, importing find_package and pulling in the source are exklusive to each other. With this change they would behave identical. If they have somehow imported an installed Release and master at the Same time, I guess the break because of the alias would be their littlest Problem.

This works in cryptopp-cmake, libupnp and even qt, so I can't Imagine people could complain about this.

[hartwork]

Given the apparent need for conditional add_subdirectory, the add_library alias call does not seem like much of a deal to me personally.

As fetchcontent_makeavailable does the add_subdirectory after find_package did the if, the only thing left ist the alias, that's the reason for this change.

@Vollstrecker I cannot confirm that add_subdirectory would be happing automatically for me: Adding FIND_PACKAGE_ARGS NAMES nosuchthing to sabotage for the sake of debugging shows that…

• with explicit add_subdirectory("${expat_SOURCE_DIR}/expat") Expat gets built and ldd will show that the self-built copy is used while
• without it CMake will just do plain -lexpat while linking, use the system Expat, and not build the cloned Expat and not use anything from find_package due to nosuchthing. That seems kind of broken and working by accident.

So the add_subdirectory cannot be omitted as to what I can observe on my own box. What am I missing, how does it do work for you?

I should note that adding add_library(expat::expat ALIAS expat) on Expat's side now will break any build systems that already define that alias without an if(NOT TARGET expat::expat) [..] endif() guard around on their side (including my own example above), with this error (with e.g. CMake 3.29.3):

Sure, but this means someone ist pulling in head unconditionally and a) hopes for the best which never is a good Idea or b) they want to See If Something Breaks to have time to react before the Release.

I'm with you that following master is a guarantee for breakage — that was not the scenario I had in mind. What we have here is that once someone does a controlled update of their Expat FetchContent SHA1, they will (a) have to try the troubling paths of the two and do fixing or (b) not notice and be broken until someone notices. Both scenarios could be considered annoying.

As you already noticed, importing find_package and pulling in the source are exclusive to each other. With this change they would behave identical.

With regard to the target name: yes; the two paths still have plenty of differences which e.g. becomes visible when dumping all variables known to CMake:

With FIND_PACKAGE_ARGS NAMES nosuchthing i.e. find_package failing:

-- expat_BINARY_DIR=/tmp/tmp.KddHwYUr3E/_deps/expat-build
-- expat_CONSIDERED_CONFIGS=
-- expat_CONSIDERED_VERSIONS=
-- expat_DIR=/tmp/tmp.KddHwYUr3E/CMakeFiles/pkgRedirects
-- expat_FOUND=0
-- expat_POPULATED=True
-- expat_SOURCE_DIR=/tmp/tmp.KddHwYUr3E/_deps/expat-src

With plain FIND_PACKAGE_ARGS i.e. find_package working:

-- _expat_config_included=TRUE
-- expat_CONFIG=/usr/lib64/cmake/expat-2.6.3/expat-config.cmake
-- expat_CONSIDERED_CONFIGS=/usr/lib64/cmake/expat-2.6.3/expat-config.cmake
-- expat_CONSIDERED_VERSIONS=2.6.3
-- expat_DIR=/usr/lib64/cmake/expat-2.6.3
-- expat_FOUND=1
-- expat_POPULATED=1
-- expat_VERSION=2.6.3
-- expat_VERSION_COUNT=3
-- expat_VERSION_MAJOR=2
-- expat_VERSION_MINOR=6
-- expat_VERSION_PATCH=3
-- expat_VERSION_TWEAK=0
-- expat_attr_info_FOUND=OFF
-- expat_char_FOUND=ON
-- expat_context_bytes_FOUND=ON
-- expat_dtd_FOUND=ON
-- expat_large_size_FOUND=OFF
-- expat_min_size_FOUND=OFF
-- expat_ns_FOUND=ON
-- expat_ushort_FOUND=OFF
-- expat_wchar_t_FOUND=OFF

This works in cryptopp-cmake, libupnp and even qt, so I can't Imagine people could complain about this.

cryptopp-cmake does have a guard before defining the alias — https://github.com/noloader/cryptopp-cmake/blob/169d9a4c4c573b062e33e4fda51ac10fb2509c24/CMakeLists.txt#L1205-L1209 — so it would work because they invested in protection. For Qt I did not easily find the right spot.

I will have to sleep over this more.

[Vollstrecker]

Now, I'm at least on a bigger display.

First thing:

cmake_minimum_required(VERSION 3.24)

include(FetchContent)

project(hello VERSION 1.0.0)

FetchContent_Declare(
expat
GIT_REPOSITORY https://github.com/libexpat/libexpat/
GIT_TAG 88b3ed5 # i.e. R_2_6_3
FIND_PACKAGE_ARGS
)

FetchContent_MakeAvailable(expat)

if(NOT expat_FOUND) # with regard to find_package
add_subdirectory("${expat_SOURCE_DIR}/expat")

As you see by yourself, you need the expat-path. fetchcontent does add_subdirectory, but in this case there is no CMakeLists.txt in there. If you add 'SOURCE_SUBDIR expat' to the declare, it works.

Second, including your sourcetree doesn't set expat_FOUND as long as you don't do that in your CMakelists.txt, same as with the alias.

Third, Findpackage_ARGS is the find-module fetchcontent should try first and if not successfull it downloads the source. For the other way around you want OVERRIDE_FIND_PACKAGE, then you get the source if you use find_package(expat) and nothing is found.

Fourth: you found the old cryptopp where everything from 2.very.old should be supported, follow the link in the README.md and you'll find this line. Btw. The line you've found was to work around old cmakes that don't create the alias (like you atm.) for a third party lib (not cryptopp), what is stated at the comment above.

But hey, if you want a guarded version, feel free. Maybe with a message stating that if someone sees the message he should open an issue and post a link to his project, and if the usage is correct I owe you a box of german beer of your choice (or of mine if you don't know the good ones).

[hartwork]

As you see by yourself, you need the expat-path. fetchcontent does add_subdirectory, but in this case there is no CMakeLists.txt in there. If you add 'SOURCE_SUBDIR expat' to the declare, it works.

@Vollstrecker I confirm that SOURCE_SUBDIR expat works:

cmake_minimum_required(VERSION 3.24)

include(FetchContent)

project(hello VERSION 1.0.0)

FetchContent_Declare(
    expat
    GIT_REPOSITORY https://github.com/libexpat/libexpat/
    GIT_TAG        88b3ed553d8ad335559254863a33360d55b9f1d6  # i.e. R_2_6_3
    SOURCE_SUBDIR  expat/
    FIND_PACKAGE_ARGS
)

FetchContent_MakeAvailable(expat)

add_executable(hello
    hello.c
)

target_link_libraries(hello PUBLIC expat)

Second, including your sourcetree doesn't set expat_FOUND as long as you don't do that in your CMakelists.txt, same as with the alias.

I fail to make sense of that statement. Could you rephrase it to help me understand? expat_FOUND seems set by find_package if I'm not mistaken.

Third, Findpackage_ARGS is the find-module fetchcontent should try first and if not successfull it downloads the source.

Yes.

For the other way around you want OVERRIDE_FIND_PACKAGE, then you get the source if you use find_package(expat) and nothing is found.

In my practical test I always got the source with OVERRIDE_FIND_PACKAGE and never the system package (and most of the find_package arguments were ignored — it's pretty weird). Please try with…

cmake_minimum_required(VERSION 3.24)

include(FetchContent)

project(hello VERSION 1.0.0)

FetchContent_Declare(
    expat
    GIT_REPOSITORY https://github.com/libexpat/libexpat/
    GIT_TAG        88b3ed553d8ad335559254863a33360d55b9f1d6  # i.e. R_2_6_3
    SOURCE_SUBDIR  expat/
    OVERRIDE_FIND_PACKAGE
)

FetchContent_MakeAvailable(expat)

find_package(expat 2.2.8 CONFIG REQUIRED char dtd ns)

add_executable(hello
    hello.c
)

target_link_libraries(hello PUBLIC expat)

…to see it with your own eyes. Same after dropping any to all of 2.2.8 CONFIG REQUIRED char dtd ns — it's not that part.

Fourth: you found the old cryptopp where everything from 2.very.old should be supported, follow the link in the README.md and you'll find this line. Btw. The line you've found was to work around old cmakes that don't create the alias (like you atm.) for a third party lib (not cryptopp), what is stated at the comment above.

I confirm that your link adds an alias the same way that you're suggesting here.

But hey, if you want a guarded version, feel free. Maybe with a message stating that if someone sees the message he should open an issue and post a link to his project, and if the usage is correct I owe you a box of german beer of your choice (or of mine if you don't know the good ones).

The guard would need to be around the second call but ours — the one suggested to introduce here — would be the first, so we cannot do anything about that, correct?

[Vollstrecker]

Second, including your sourcetree doesn't set expat_FOUND as long as you don't do that in your CMakelists.txt, same as with the alias.

I fail to make sense of that statement. Could you rephrase it to help me understand? expat_FOUND seems set by find_package if I'm not mistaken.

If found: yes, but your example did add_subdirectory and then if(NOT expat_FOUND) and never called find_package.

For the other way around you want OVERRIDE_FIND_PACKAGE, then you get the source if you use find_package(expat) and nothing is found.

In my practical test I always got the source with OVERRIDE_FIND_PACKAGE and never the system package (and most of the find_package arguments were ignored — it's pretty weird). Please try with…

From the docs:
When a FetchContent_Declare( ...) call includes this option, subsequent calls to find_package( ...) will ensure that FetchContent_MakeAvailable() has been called, then use the config package files in the CMAKE_FIND_PACKAGE_REDIRECTS_DIR directory (which are usually created by FetchContent_MakeAvailable()). This effectively makes FetchContent_MakeAvailable() override find_package() for the named dependency, allowing the former to satisfy the package requirements of the latter. FIND_PACKAGE_ARGS cannot be used when OVERRIDE_FIND_PACKAGE is given.

But hey, if you want a guarded version, feel free. Maybe with a message stating that if someone sees the message he should open an issue and post a link to his project, and if the usage is correct I owe you a box of german beer of your choice (or of mine if you don't know the good ones).

The guard would need to be around the second call but ours — the one suggested to introduce here — would be the first, so we cannot do anything about that, correct?

Nope: The call would be the second one if the first one is find_package (that succeeds) and then the source is pulled without checking the result. Or if the source is pulled with another name a second time, but that would be plain bullshit.

[hartwork]

Second, including your sourcetree doesn't set expat_FOUND as long as you don't do that in your CMakelists.txt, same as with the alias.

I fail to make sense of that statement. Could you rephrase it to help me understand? expat_FOUND seems set by find_package if I'm not mistaken.

If found: yes, but your example did add_subdirectory and then if(NOT expat_FOUND) and never called find_package.

@Vollstrecker I think we're both referring to my example from #903 (comment) here, correct?
The core of it was:

FetchContent_Declare(
    expat
    GIT_REPOSITORY https://github.com/libexpat/libexpat/
    GIT_TAG        88b3ed553d8ad335559254863a33360d55b9f1d6  # i.e. R_2_6_3
    FIND_PACKAGE_ARGS
)

FetchContent_MakeAvailable(expat)

if(NOT expat_FOUND)  # with regard to find_package
    add_subdirectory("${expat_SOURCE_DIR}/expat")
    add_library(expat::expat ALIAS expat)
endif()

Here FetchContent_Declare is calling find_package internally (assuming default FETCHCONTENT_TRY_FIND_PACKAGE_MODE at OPT_IN) because FIND_PACKAGE_ARGS is given. That's how expat_FOUND get's defined in any case.

For the other way around you want OVERRIDE_FIND_PACKAGE, then you get the source if you use find_package(expat) and nothing is found.

In my practical test I always got the source with OVERRIDE_FIND_PACKAGE and never the system package (and most of the find_package arguments were ignored — it's pretty weird). Please try with…

From the docs: When a FetchContent_Declare( ...) call includes this option, subsequent calls to find_package( ...) will ensure that FetchContent_MakeAvailable() has been called, then use the config package files in the CMAKE_FIND_PACKAGE_REDIRECTS_DIR directory (which are usually created by FetchContent_MakeAvailable()). This effectively makes FetchContent_MakeAvailable() override find_package() for the named dependency, allowing the former to satisfy the package requirements of the latter. FIND_PACKAGE_ARGS cannot be used when OVERRIDE_FIND_PACKAGE is given.

There is nothing in there claiming that OVERRIDE_FIND_PACKAGE would make find_package ever prefer the system package if available that I could see — you? When I was reading these docs last time we talked I was indeed expecting it to prefer the system Expat but I guess that's wishful thinking: the docs don't seem to say that. I tried multiple times but with OVERRIDE_FIND_PACKAGE system Expat is never used on my Linux box: it's always Expat from Git. If you observe otherwise, please provide a minimal reproducer, the core of mine is:

FetchContent_Declare(
    expat
    GIT_REPOSITORY https://github.com/libexpat/libexpat/
    GIT_TAG        88b3ed553d8ad335559254863a33360d55b9f1d6  # i.e. R_2_6_3
    SOURCE_SUBDIR  expat/
    OVERRIDE_FIND_PACKAGE
)

FetchContent_MakeAvailable(expat)

find_package(expat)

But hey, if you want a guarded version, feel free. Maybe with a message stating that if someone sees the message he should open an issue and post a link to his project, and if the usage is correct I owe you a box of german beer of your choice (or of mine if you don't know the good ones).

The guard would need to be around the second call but ours — the one suggested to introduce here — would be the first, so we cannot do anything about that, correct?

Nope: The call would be the second one if the first one is find_package (that succeeds) and then the source is pulled without checking the result.

I see.

---

Having thought about this pull request more I come to this conclusion:

• The gain is allowing to link against target expat::expat in two different cases — find_package in config mode and flavors of FetchContent_Declare — without needing to care or check for the difference on user side: things become more simple on the surface and more convenient.
• I personally consider the gain minimal, and the risk non-zero. Not a combination that I'd be in favor of in general.
• I understand that use of FetchContent_Declare with a moving Git target is asking for trouble by design, and hence we can risk breaking that scenario if existing; unlike the library, the Expat build system does guarantee backwards-compatiblity across releases, and these users and projects will "be okay".
• A guard on our side helps cases that arguably should just fail with an error.
• The suggested change seems to be a two-way door — we can revert it for the next or second-next release if it causes more trouble than expected in the meantime.
• I will follow your suggestion of defining target expat::expat for FetchContent_Declare and merge this pull request now.