Replace tinyxml2_LIBRARY with tinyxml2_LIBRARIES by gdsotirov · Pull Request #3897 · danmar/cppcheck

gdsotirov · 2022-03-13T20:33:49Z

I was not able to report an issue, so I use this pull request to report the problem and hopefully solve it together.

Building cppcheck 2.7 with system's tinyxml2 library (i.e. setting USE_BUNDLED_TINYXML2=OFF) fails with the following:

CMake Error at gui/test/benchmark/simple/CMakeLists.txt:4 (add_executable):
  Error evaluating generator expression:

    $<TARGET_OBJECTS:tinyxml2_objs>

  Objects of target "tinyxml2_objs" referenced but no such target exists.

I found and applied the changes from commit 974dd5d (which was made the day after 2.7 release BTW), but build still fails for "undefined symbols" from TinyXML2, because the library is not linked (no -ltinyxml2) to several targets. The reason for this seams to be that variable tinyxml2_LIBRARY is not defined (see cmake/findDependencies.cmake:51).

I therefore simply replaced the variable just to be able to build. However, the correct solution should be to properly define tinyxml2_LIBRARY for which I think find_library(tinyxml2_LIBRARY tinyxml2) should be called also when tinyxml2 package is found. Let me know what you think and I'll update the PR.

firewave · 2022-03-13T22:25:07Z

Will take a proper look tomorrow.

It seems like the variable is actually called tinyxml2_LIBRARIES when the package is found. Could you verify that on your end?

I assume my platform does not supply the pkg-config for tinyxml2 so it fell through to the find_library() and that's why the build worked for me. Probably also the case in our CI.

firewave · 2022-03-14T13:27:13Z

It seems I was correct about the incorrect variable name.

From https://cmake.org/cmake/help/latest/module/FindPkgConfig.html:

<XXX>_LINK_LIBRARIES
the libraries and their absolute paths

So if you could adjust your patch to just replace tinyxml2_LIBRARY with tinyxml2_LIBRARIES everywhere we have a proper fix. 🙂

gdsotirov · 2022-03-14T13:48:19Z

Yes, I have pkg-config for tinyxml2 on my build platform, so it was not going to find_library at all. However, If I replace tinyxml2_LIBRARY with tinyxml2_LIBRARIES would not it break all platforms without pkg-config support? As far as I understand find_library would only set tinyxml2_LIBRARY if found. Perhaps the result of either find_package or find_library should got into a common variable that could be used in target_link_libraries to set properly linker flags?

firewave · 2022-03-14T14:45:58Z

find_library() sets the variable you pass it and we simply used the wrong one so the fallback didn't really work. Also if the tinyxml2 library is not found at all we bail out with message(FATAL_ERROR).

Perhaps the result of either find_package or find_library should got into a common variable that could be used in target_link_libraries to set properly linker flags?

The "common" variable is tinyxml2_LIBRARIES.

This was just a simple typo which never caused an issue so far since we didn't actually used the library variable for target_link_libraries() before.

I found and applied the changes from commit 974dd5d (which was made the day after 2.7 release BTW)

This was just reported by several package maintainers after the release.

gdsotirov · 2022-03-14T15:06:00Z

OK. I got your point. In fact I got confused, because find_package is for external projects, not for pkg-config (e.g. pkg_search_module), so find_library was not skipped. I just finished testing the changes and I'll push them shortly.

gdsotirov · 2022-03-14T15:08:58Z

This was just reported by several package maintainers after the release.

I'm package maintainer myself :-) I just meant that the commit missed the release, but I hope there would be another a new one soon, so we (maintainers) do not have to deal with patches.

firewave · 2022-03-14T15:21:51Z

I'm package maintainer myself :-) I just meant that the commit missed the release, but I hope there would be another a new one soon, so we (maintainers) do not have to deal with patches.

@danmar sometimes does point releases (and he already did 2.7.1) so maybe this and 974dd5d could be cherry-picked into the next one.

So if you could adjust your patch to just replace tinyxml2_LIBRARY with tinyxml2_LIBRARIES everywhere we have a proper fix. 🙂

You forgot to adjust the files in the cmake subfolder.

gdsotirov · 2022-03-14T15:26:01Z

You forgot to adjust the files in the cmake subfolder.

I only found the tinyxml2_LIBRARY variable used in cmake/printInfo.cmake and replaced it. Or you meant something else?

firewave · 2022-03-14T16:27:01Z

cmake/printInfo.cmake

 message( STATUS "USE_BUNDLED_TINYXML2 =  ${USE_BUNDLED_TINYXML2}" )
 if (NOT USE_BUNDLED_TINYXML2)
-    message(STATUS "tinyxml2_LIBRARY =       ${tinyxml2_LIBRARY}")
+    message(STATUS "tinyxml2_LIBRARY =       ${tinyxml2_LIBRARIES}")


tinyxml2_LIBRARY -> tinyxml2_LIBRARIES

firewave · 2022-03-14T16:28:19Z

I only found the tinyxml2_LIBRARY variable used in cmake/printInfo.cmake and replaced it. Or you meant something else?

The find_library() call and check in cmake/findDependencies.cmake.

danmar · 2022-03-14T18:18:14Z

@danmar sometimes does point releases (and he already did 2.7.1) so maybe this and 974dd5d could be cherry-picked into the next one.

Releasing a fixed 2.7.2 sounds good to me.

firewave · 2022-03-15T11:51:17Z

The find_library() call and check in cmake/findDependencies.cmake.

Still missing that change in that file - hence the build failures.

gdsotirov · 2022-03-15T17:40:49Z

OK. I had to dug deeper to understand the problem.

First, yesterday when I tested the changes I forgot to clear CMake cache, so the successful result was bogus. My bad, lesson learned.

Then, it turns out TinyXML2 installs tinyxml2-config.cmake in /usr/lib64/cmake/tinyxml2, which after find_package sets variable tinyxml2_FOUND - a problem of it's own. However, like this in my case find_library was thus not called.

A normal find_package should set tinyxml2_LIBRARIES variable properly, but if not this should be done by find_library. I now understand you @firewave and the requested change is made. In fact it should be just like for Z3 library above.

firewave · 2022-03-15T18:17:10Z

In fact it should be just like for Z3 library above.

Yup. Thanks.

Will merge if the build is successful. But I guess we should adjust the title of the PR before so it actually reflects what was done.

gdsotirov · 2022-03-15T18:25:03Z

The builds should pass. Please, adjust the title as you find appropriate.

See danmar/cppcheck#3897

Do not use tinyxml2_LIBRARY as it is not defined

b8b3511

firewave self-assigned this Mar 13, 2022

Replace tinyxml2_LIBRARY with tinyxml2_LIBRARIES

f245dd9

firewave reviewed Mar 14, 2022

View reviewed changes

Change variable name also in message text

056b558

Rename tinyxml2_LIBRARY to tinyxml2_LIBRARIES

e5c8d27

gdsotirov mentioned this pull request Mar 15, 2022

Installed tinyxml2-config.cmake confuses library detection leethomason/tinyxml2#904

Closed

gdsotirov changed the title ~~Do not use tinyxml2_LIBRARY as it is not defined (or define it properly?)~~ Replace tinyxml2_LIBRARY with tinyxml2_LIBRARIES Mar 15, 2022

firewave merged commit 33315b0 into danmar:main Mar 15, 2022

firewave pushed a commit to firewave/cppcheck that referenced this pull request Mar 18, 2022

Replace tinyxml2_LIBRARY with tinyxml2_LIBRARIES (danmar#3897)

dd32660

danmar pushed a commit that referenced this pull request Mar 19, 2022

Replace tinyxml2_LIBRARY with tinyxml2_LIBRARIES (#3897)

199fe34

gdsotirov added a commit to gdsotirov/cppcheck.SlackBuild that referenced this pull request Mar 19, 2022

Bump version to 2.7.3, remove patch

f0b39c7

See danmar/cppcheck#3897

c72578 mentioned this pull request Mar 20, 2022

Set tinyxml2_LIBRARIES after find_package() #3918

Merged

Conversation

gdsotirov commented Mar 13, 2022

Uh oh!

firewave commented Mar 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

firewave commented Mar 14, 2022

Uh oh!

gdsotirov commented Mar 14, 2022

Uh oh!

firewave commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gdsotirov commented Mar 14, 2022

Uh oh!

gdsotirov commented Mar 14, 2022

Uh oh!

firewave commented Mar 14, 2022

Uh oh!

gdsotirov commented Mar 14, 2022

Uh oh!

firewave Mar 14, 2022

Choose a reason for hiding this comment

Uh oh!

gdsotirov Mar 14, 2022

Choose a reason for hiding this comment

Uh oh!

firewave commented Mar 14, 2022

Uh oh!

danmar commented Mar 14, 2022

Uh oh!

firewave commented Mar 15, 2022

Uh oh!

gdsotirov commented Mar 15, 2022

Uh oh!

firewave commented Mar 15, 2022

Uh oh!

gdsotirov commented Mar 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

firewave commented Mar 13, 2022 •

edited

Loading

firewave commented Mar 14, 2022 •

edited

Loading