Skip to content

CMake fixes#44

Merged
robUx4 merged 2 commits intoMatroska-Org:masterfrom
evpobr:cmake
May 19, 2019
Merged

CMake fixes#44
robUx4 merged 2 commits intoMatroska-Org:masterfrom
evpobr:cmake

Conversation

@evpobr
Copy link
Copy Markdown
Contributor

@evpobr evpobr commented May 5, 2019

  • Adds ebml exported target to EBML:: namespace as recommended
  • Set uniform ${CMAKE_PACKAGE_INSTALLDIR} to make it simpler
  • Properly set EBML_FOUND variable when EBML is searched in config mode

@mbunkus
Copy link
Copy Markdown
Contributor

mbunkus commented May 17, 2019

Thanks. After merging your other PR, this one doesn't apply anymore. Please rebase and push.

Apart from that, I don't know enough about how cmake works in order to assess how the changes affect existing programs. Can you please elaborate on what exactly they do? For example, if you have an existing program linked to libMatroska & libEBML and you update both libraries with your proposed changes without recompiling that program, would it still work with the new library versions (= would these changes break the ABI)?

And do these changes break the API (meaning would changes have to be made to the source code or the build system of a program using libMatroska/libEBML with your proposed changes applied)?

The same questions apply to your PR against libMatroska.

@evpobr
Copy link
Copy Markdown
Contributor Author

evpobr commented May 17, 2019

Thanks. After merging your other PR, this one doesn't apply anymore. Please rebase and push.

Done.

Apart from that, I don't know enough about how cmake works in order to assess how the changes affect existing programs. Can you please elaborate on what exactly they do? For example, if you have an existing program linked to libMatroska & libEBML and you update both libraries with your proposed changes without recompiling that program, would it still work with the new library versions (= would these changes break the ABI)?

No API or ABI chages. This fixes CMake script only.

And do these changes break the API (meaning would changes have to be made to the source code or the build system of a program using libMatroska/libEBML with your proposed changes applied)?

Yes. If somebody uses EBML config package module (Windows users mostly, Unix users have PkgConfig), they need to make trivial changes in their CMake script:

target_link_libraries(helloworld PRIVATE ebml)

to

target_link_libraries(helloworld PRIVATE EBML::ebml)

I think it is better to fix it now to make CMake closer to standards (that EBML:: namespace is recommended to use):

A NAMESPACE with double-colons is specified when exporting the targets for installation. This convention of double-colons gives CMake a hint that the name is an IMPORTED target when it is used by downstreams with the target_link_libraries() command. This way, CMake can issue a diagnostic if the package providing it has not yet been found.

Fix compilation of libMatroska.
@evpobr
Copy link
Copy Markdown
Contributor Author

evpobr commented May 18, 2019

Iguess b7acd9d is not API break, these members are exported anyway because their classes are exported.

@mbunkus
Copy link
Copy Markdown
Contributor

mbunkus commented May 19, 2019

OK, thanks. Unfortunately I cannot test the changes to the DLL export/import mechanism as I only link Windows stuff statically in my projects. @robUx4 can you give this one and the corresponding Matroska PR a try, please?

@mbunkus mbunkus requested a review from robUx4 May 19, 2019 09:04
Copy link
Copy Markdown
Contributor

@robUx4 robUx4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patchset seem OK. I can't comment on the installed CMake files for other projects to include but it seems OK.

@evpobr
Copy link
Copy Markdown
Contributor Author

evpobr commented May 19, 2019

Vpkg ports update: microsoft/vcpkg#6526

@evpobr evpobr deleted the cmake branch May 19, 2019 16:28
evpobr added a commit to evpobr/libebml that referenced this pull request May 21, 2019
Unix paths are case sensitive, but we use EbmlConfig.cmake.in & EBMLConfig.cmake.in.

Related to Matroska-Org#44, microsoft/vcpkg#6526
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants