Skip to content

Cmake improvements#91

Merged
dariusz-f merged 12 commits intoAcademySoftwareFoundation:mainfrom
BtbN:cmake_improvements
May 23, 2025
Merged

Cmake improvements#91
dariusz-f merged 12 commits intoAcademySoftwareFoundation:mainfrom
BtbN:cmake_improvements

Conversation

@BtbN
Copy link
Contributor

@BtbN BtbN commented May 9, 2025

These are some things I ran across while adding support to my FFmpeg builds.
Almost all of these clean up/enhance the CMake build system, the last commit puts the visibility/export macro in the right place, to make it actually work and fix the corresponding warning:

[ 86%] Building C object src/CMakeFiles/oapv_dynamic.dir/avx/oapv_tq_avx.c.o
In file included from /home/btbn/Projects/openapv/src/./oapv_def.h:43,
from /home/btbn/Projects/openapv/src/avx/oapv_tq_avx.c:32:
/home/btbn/Projects/openapv/src/../inc/oapv.h:661:1: warning: ‘visibility’ attribute ignored on non-class types [-Wattributes]
661 | char * OAPV_EXPORT oapv_version();
| ^~~~

@jamrial
Copy link
Contributor

jamrial commented May 9, 2025

Project version and library/soname version don't necessarily have to match. The latter is only bumped when you want to tag a new release and is mostly cosmetic, but the latter needs to be bumped every time the API/ABI changes (Major for non-backwards compatible changes/removals, minor for backwards compatible additions).

@BtbN
Copy link
Contributor Author

BtbN commented May 10, 2025

It's still odd and rather unusual to see the library names be "lib.so", "lib.so.0.1" and "lib.so.1". I don't think I've ever seen that.
I also don't think it's technically an issue, since I'm not aware of anything ever using the .0.1 one. So it is purely cosmetic.

@jamrial
Copy link
Contributor

jamrial commented May 10, 2025

It's still odd and rather unusual to see the library names be "lib.so", "lib.so.0.1" and "lib.so.1". I don't think I've ever seen that. I also don't think it's technically an issue, since I'm not aware of anything ever using the .0.1 one. So it is purely cosmetic.

Yes, that's wrong and needs to be fixed. I was just commenting Re #86 since this PR was linked there.

@BtbN
Copy link
Contributor Author

BtbN commented May 10, 2025

Oh, that's not something addressed here.
I can look into it though.

@BtbN BtbN force-pushed the cmake_improvements branch 2 times, most recently from 9d18e77 to c7b07f0 Compare May 10, 2025 17:56
@BtbN
Copy link
Contributor Author

BtbN commented May 10, 2025

Added such a header now. This should fullfill what you're looking for?

@jamrial
Copy link
Contributor

jamrial commented May 10, 2025

As is, PROJECT_VERSION_MAJOR is 0 but the library soname major is 1, if I'm not mistaken.
The header defines should reflect the library soname versions, so i think there should be a separation between ABI and project versioning.

But yes, this is in the right direction. Thanks.

@BtbN
Copy link
Contributor Author

BtbN commented May 10, 2025

Right now the soversion is just 1, while the project major is 0. While unusual, it doesn't immediately strike me as a problem.

@jamrial
Copy link
Contributor

jamrial commented May 10, 2025

Project major being 0 is fine, but the public define in the header reflecting the project major and not the soname major is a problem. Library users care about API availability, and they check the public header for it. This PR right now emits

#define OAPV_VERSION_MAJOR 0
#define OAPV_VERSION_MINOR 1
#define OAPV_VERSION_PATCH 13

As in, no library user checks for ffmpeg 7.x as it's purely a cosmetic tag for packagers and not reflected anywhere in the headers. They check for lavc 61.x

@BtbN
Copy link
Contributor Author

BtbN commented May 10, 2025

So you want to set project major == soversion?
I'd assume that when the soname is ever bumped, the major version would be as well, or at least some other version, meaning it can be checked for.
So I don't see an issue with the two being disconnected. With the defines you check for APIs, while the soname respresents the ABI version. The later is not normally of concern at the API level.

@jamrial
Copy link
Contributor

jamrial commented May 10, 2025

So you want to set project major == soversion?

No, i want the header define to reflect soname major and not project major.

The reason is, this project is clearly using the project version for tagging purposes (as seen in https://github.com/AcademySoftwareFoundation/openapv/tags), bumping them when they make a new release. API additions should be reflected in both header and soname with a minor bump when they happen, not when a release is tagged.

So I don't see an issue with the two being disconnected

Them being disconnected is fine and expected. My point is that the headers should reflect one and not the other.

@jamrial
Copy link
Contributor

jamrial commented May 10, 2025

So, in short, to start, project version 0.1.13 (current), soname 1.0.0 (I think?)

On API additions, soname minor is bumped, and this is reflected in the header. Project version is unchanged until a new tag is made, where they can bump it to whatever arbitrary number they want, and this is not reflected in the header.

@BtbN
Copy link
Contributor Author

BtbN commented May 10, 2025

The soversion comes from right here: https://github.com/AcademySoftwareFoundation/openapv/blob/main/src/CMakeLists.txt#L3
It'll always stay that until someone changes that line.

Right now, here: https://github.com/AcademySoftwareFoundation/openapv/blob/main/src/CMakeLists.txt#L52
It sets project major and minor as version (not soversion) of the library. After this PR, it will be soversion.major.minor as library version, so the two are aligned.

The only thing not accessible in the header is the soversion, which should not ever be important for the code anyway, since it'll only be bumped very infrequently, when breaking API changes occur.
The project version derived from git tags or version.txt is what's in the header. There is no other version I'd know that could be put there.

@jamrial
Copy link
Contributor

jamrial commented May 10, 2025

It sets project major and minor as version (not soversion) of the library. After this PR, it will be soversion.major.minor as library version, so the two are aligned.

So it will be liboapv.so.1.0.1, and if the next tagged release is for example 0.1.14, that soversion will not change, even if there's new API.
My suggestion is something like this: jamrial@4eefe18 to separate project from soname.

@BtbN
Copy link
Contributor Author

BtbN commented May 10, 2025

The soname won't change ever anyway, it's just the project name.
Neither will the soversion, which is just 1.
That other just "version", without leading "so" isn't used by anything as far as I'm aware. Including the patch level at the end would be simple enough, but I'm not sure what it achieves?

Both your approach and the current one will depend on the proper maintenance of either tags or those entries in the cmakelist.

@jamrial
Copy link
Contributor

jamrial commented May 11, 2025

Neither will the soversion, which is just 1.

Why would it not change? If any exported function is removed, or the offset and/or size of fields within a public struct are changed, they need to bump major soversion. Otherwise, existing binaries that linked to it will crash.

That other just "version", without leading "so" isn't used by anything as far as I'm aware. Including the patch level at the end would be simple enough, but I'm not sure what it achieves?

Not sure what other "version" you're talking about. If you mean the VERSION_{MAJOR,MINOR,PATCH} ones, those are set to the value in the git tag or version.txt, and passed to CMake to set the global standard defines PROJECT_VERSION_{MAJOR/MINOR/PATCH}. They are equal and interchangeable, afaik.
In my patch, i replace LIB_SOVERSION with LIB_SOVERSION_{MAJOR,MINOR,PATCH}, meant for versioning the library soname in a way independent from the project version.

Both your approach and the current one will depend on the proper maintenance of either tags or those entries in the cmakelist.

Well, proper maintenance is expected of any project that intends to be added to any package manager or included as dependency in another project. This one isn't excluded.
The tags are arbitrary. Just like it's 0.1.13 now the next could be 0.1.14 or 0.2.0, and it reflects nothing other than the perception that the project had enough changes to guarantee a big enough number increase. But library soversions that follow semver have a real purpose for the sake of binaries linking to your library ("Are these sizes and offsets as i expect them?", "Is this symbol present?", etc), and are not cosmetic. That's why they should be separate.

@BtbN
Copy link
Contributor Author

BtbN commented May 11, 2025

There are two distinct versions set on the library. The soversion, which is just one single number.
This is what needs bumped on an ABI break, so prevent bad runtime behaviour.

The other is what cmake calls just "version". It controls the numbers in the other symlink. But I'm still not aware of that symlink being used for anything, so what numbers are shown there are largely inconsequential.
So what you're looking for is already in place: When ABI breaking changes are made, care has to be taken to bump the LIB_SOVERSION in src/CMakeLists.txt

Any other version, which is currently derived from tags, is more of interest to check if a certain API might exist at compile time.
It has no impact on the soversion. But it does go into said plain version of the library, where any change is inconsequential.

@dkozinski
Copy link
Collaborator

dkozinski commented May 12, 2025

I will systematize certain things.

VERSION: This typically refers to the full version of the library and is expressed in a format like major.minor.patch (e.g., 0.1.13). It indicates the specific release of the library, including all changes, bug fixes, and new features.

SOVERSION: This should be an integer that represents the ABI version and is used to indicate whether changes in the library are backward incompatible.

  • This is an integer that represents the ABI version of the library.
  • It indicates whether changes in the library are backward incompatible.
  • When you change SOVERSION, you inform users that there may be compatibility issues, which is crucial when managing dependencies in projects.

In summary, SOVERSION should be a simple integer and not a full version number in the major.minor.patch format.

Currently

Currently, the names of the links to the openAPV shared library contain both the library full VERSION and the SOVERSION. It looks as follows:

1. liboapv.so:

  • This is the generic shared library name that points to the current version of the library.
  • It links to the latest SOVERSION ( liboapv.so -> liboapv.so.1 )

2. liboapv.so.1:

  • This file represents the SOVERSION, which is 1 in this case.
  • It links to the specific version of the library that corresponds to the full library version, in our case, 0.1.13 (it does not correspond to the ABI version).
    ( liboapv.so.1 -> liboapv.so.0.1 )

3. liboapv.so.0.1:

  • This file represents the full version of the library, where VERSION is 0.1.13. and it includes the major and minor numbers.

SOVERSION = 1
VERSION.major = 0
VERSION.minor = 1
VERSION.patch = 13

Proposed changes

What can we do with this?

1. Decouple the library versioning from SOVERSION (complete independence of VERSION from SOVERSION).

As proposed by Jamrial, decouple the library versioning from the ABI version.

Results:

1.1. liboapv.so

  • generic shared library name that points to the current version of the library. It links to the latest SOVERSION.

1.2. liboapv.so.1

  • it represents the SOVERSION, which is 1 in this case. It links to the specific version of the library that corresponds to the ABI version.

1.3. liboapv.so.1.0.1
This file represents the full ABI version of the library, where ABI_VERSION is, let's say, 1.1.0. [LIB_SOVERSION_MAJOR, LIB_SOVERSION_MINOR, LIB_SOVERSION_PATCH 0] This includes the major and minor numbers.

2. Make VERSION.major dependent on SOVERSION (Dependency of the minor component of VERSION on SOVERSION).

VERSION.major = SOVERSION

With each change to the public API (changes in the API interface that are incompatible with previous versions) that may cause backward compatibility issues (i.e removal of functions, change of function signatures), change VERSION.minor and SOVERSION (VERSION.minor = SOVERSION).

Results:

2.1. liboapv.so

  • generic shared library name that points to the current version of the library. It links to the latest SOVERSION.

2.2. liboapv.so.1

  • it represents the SOVERSION, which is 1 in this case. It links to the full version of the library that corresponds to the ABI version.

2.3. liboapv.so.1.0.13

  • This file represents the full version of the library, where VERSION is, say, 1.1.13 [VERSION.major, VERSION.minor, VERSION.patch].

This includes the major and minor numbers.

3. Last option: leave it as it is

Optionally, we can include the full version of the library 0.1.13 in the so name (liboapv.so.0.1.13 instead of liboapv.so.0.1)

1. liboapv.so:

  • This is the generic shared library name that points to the current version of the library.
  • It links to the latest SOVERSION ( liboapv.so -> liboapv.so.1 )

2. liboapv.so.1:

  • This file represents the SOVERSION, which is 1 in this case.
  • It links to the specific version of the library that corresponds to the full library version, in our case, 0.1.13 (it does not correspond to the ABI version).
    ( liboapv.so.1 -> liboapv.so.0.1.13 )

3. liboapv.so.0.1.13:

  • This file represents the full version of the library, where VERSION is 0.1.13. and it includes the major and minor numbers.

@dkozinski
Copy link
Collaborator

Personally, I opt for the solution no 1 presented by jamrial (jamrial@4eefe18) , which completely separates the versioning of the library and the ABI.

@jamrial
Copy link
Contributor

jamrial commented May 13, 2025

Personally, I opt for the solution no 1 presented by jamrial (jamrial@4eefe18) , which completely separates the versioning of the library and the ABI.

It might be a good idea to make it clear somewhere other than src/CMakeLists.txt (Is there a developer guidelines document?) that new API additions, be it new symbols, enum values or defines in public headers, require a minor bump, given that it wasn't a concern until now.

@BtbN BtbN force-pushed the cmake_improvements branch 2 times, most recently from 10f3237 to 8e4e36d Compare May 13, 2025 17:06

#define OAPV_VERSION_MAJOR @PROJECT_VERSION_MAJOR@
#define OAPV_VERSION_MINOR @PROJECT_VERSION_MINOR@
#define OAPV_VERSION_PATCH @PROJECT_VERSION_PATCH@
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point in having both project and library versions here? API users only care about the latter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @jamrial that the project version is unnecessary here.

@dkozinski
Copy link
Collaborator

Timo, please move the changes related to project configuration: shared, static to a separate PR


#define OAPV_VERSION_MAJOR @PROJECT_VERSION_MAJOR@
#define OAPV_VERSION_MINOR @PROJECT_VERSION_MINOR@
#define OAPV_VERSION_PATCH @PROJECT_VERSION_PATCH@
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @jamrial that the project version is unnecessary here.

set_target_properties(${LIB_NAME_BASE}_dynamic PROPERTIES VERSION ${PROJECT_VERSION_MAJOR}.${PROJECT_VERSION_MINOR} SOVERSION ${LIB_SOVERSION})
if(OAPV_BUILD_SHARED_LIB)
add_library(${LIB_NAME_BASE}_dynamic SHARED ${SOURCE_FILES})
set_target_properties(${LIB_NAME_BASE}_dynamic PROPERTIES VERSION ${LIB_VERSION_MAJOR}.${LIB_VERSION_MINOR}.${LIB_VERSION_PATCH} SOVERSION ${LIB_VERSION_MAJOR})
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's ok

@BtbN
Copy link
Contributor Author

BtbN commented May 14, 2025

I can make a new PR with only the first or first two commits, but I cannot easily remove them from this PR, since the changes sit on top of the previous modifications of the CMakeLists.
So this PR will then effectively depend on the new one.

Copy link
Collaborator

@dkozinski dkozinski left a comment

Choose a reason for hiding this comment

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

Please remove from this PR the changes that have already been made in #PR95.

add_definitions(-DOAPV_STATIC_DEFINE)
endif(OAPV_APP_STATIC_BUILD)

option(OAPV_BUILD_APPS "Build the included command line apps" ON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes below (lines 68-82) are already in #PR95

add_subdirectory(src)
add_subdirectory(app)

if(OAPV_BUILD_APPS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes below (lines 135-137) are already in #PR95

add_compile_definitions(_IS64BIT)
endif()

if(ARM)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes below are already in #PR95

add_library( ${LIB_NAME_BASE}_dynamic SHARED ${LIB_API_SRC} ${LIB_INC} ${LIB_BASE_SRC} ${LIB_BASE_INC}
${LIB_NEON_SRC} ${LIB_NEON_INC} )
set(SOURCE_FILES ${LIB_API_SRC} ${LIB_INC} ${LIB_BASE_SRC} ${LIB_BASE_INC} ${LIB_NEON_SRC} ${LIB_NEON_INC})
elseif(X86)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes below are already in #PR95

set(SOURCE_FILES ${LIB_API_SRC} ${LIB_INC} ${LIB_BASE_SRC} ${LIB_BASE_INC})
endif()

if(OAPV_BUILD_STATIC_LIB)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes below are already in #PR95

set(AVX ${LIB_AVX_SRC} )
set(NEON ${LIB_NEON_SRC} ${LIB_NEON_INC})

if(MSVC)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes below are already in #PR95

#
# Set DCMAKE_INSTALL_PREFIX to change default install prefix
# e.g cmake .. -DSET_PROF=BASE -DCMAKE_INSTALL_PREFIX='/home/user/bin/'
include(GNUInstallDirs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The change is already in #PR95

ARCHIVE COMPONENT Development DESTINATION ${CMAKE_INSTALL_LIBDIR}/${LIB_NAME_BASE}
PUBLIC_HEADER COMPONENT Development DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/${LIB_NAME_BASE}
)
if(OAPV_BUILD_STATIC_LIB)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes below are already in #PR95

install(TARGETS ${LIB_NAME_BASE}_dynamic
RUNTIME COMPONENT Libraries DESTINATION ${CMAKE_INSTALL_BINDIR}
LIBRARY
if(OAPV_BUILD_SHARED_LIB)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes below are already in #PR95

RUNTIME COMPONENT Libraries DESTINATION ${CMAKE_INSTALL_BINDIR}
LIBRARY
if(OAPV_BUILD_SHARED_LIB)
set_target_properties(${LIB_NAME_BASE}_dynamic PROPERTIES PUBLIC_HEADER "${OAPV_PUBLIC_HEADERS}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes below are already in #PR95

@BtbN
Copy link
Contributor Author

BtbN commented May 15, 2025

Like I said here and in #95, this PR depends on #95 to be merged first, since it sits on top of it. All the other changes to the CMakeList rather complex, so rebasing them to master would either lead to huge merge conflicts.

Once #95 is merged, the commit they share should vanish from here on its own.

@BtbN BtbN force-pushed the cmake_improvements branch from 240054c to 5b156dc Compare May 15, 2025 14:04
BtbN added 5 commits May 15, 2025 16:04
Signed-off-by: Timo Rothenpieler <timo@rothenpieler.org>
Signed-off-by: Timo Rothenpieler <timo@rothenpieler.org>
This fixes compiler warnings like:
warning: ‘visibility’ attribute ignored on non-class types [-Wattributes]

Signed-off-by: Timo Rothenpieler <timo@rothenpieler.org>
Signed-off-by: Timo Rothenpieler <timo@rothenpieler.org>
Signed-off-by: Timo Rothenpieler <timo@rothenpieler.org>
@dkozinski
Copy link
Collaborator

@BtbN I added one change to your pull request that is necessary for Android.

dariusz-f
dariusz-f previously approved these changes May 16, 2025
@BtbN BtbN force-pushed the cmake_improvements branch from f18e63d to f641654 Compare May 16, 2025 11:09
Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>
@dariusz-f
Copy link
Collaborator

We just need confirmation that on Android is it ok, after that we proceed further.

inc/oapv.h Outdated
Comment on lines 39 to 51
// The build system used in AOSP is Soong, not CMake.
//
// In the Android.bp file for the liboapv project, there are no rules to generate
// the oapv_config.h file from the oapv_config.h.in template.
//
// As a result, the oapv_config.h file is not available on the Android platform,
// and consequently, the OAPV_CHECK_LIB_VERSION macro and the OAPV_LIB_VERSION_MAJOR,
// OAPV_LIB_VERSION_MINOR, and OAPV_LIB_VERSION_PATCH definitions, which specify
// the version of the oapv library, are not accessible to Android users.
//
// To make it available, it is necessary to consider adding a mechanism in the Android.bp file
// that generates the oapv_config.h file from the oapv_config.h.in template file.
//
Copy link
Collaborator

@kpchoi kpchoi May 17, 2025

Choose a reason for hiding this comment

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

Do we need to this long comments in API header?
And, I am wondering that there might not be any issue if OAPV_LIB_VERSION_MAJOR, OAPV_LIB_VERSION_MINOR, OAPV_LIB_VERSION_PATCH are not available in Android system.

dkozinski added 2 commits May 19, 2025 09:28
…v_config.h header is not available on Android

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>
- Update oapv.h to include configuration and export headers based on OAPV_CONFIG_HEADER and OAPV_EXPORT_HEADER

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>
inc/oapv.h Outdated

/* Not available on Android due to different build system */
#ifndef __ANDROID__
#if defined(OAPV_CONFIG_HEADER)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't this lead to the header never getting included when installed, due to that define not being set anywhere but in the libraries CMakeLists?

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>
@dariusz-f
Copy link
Collaborator

@dkozinski it's not building correctly, need to fix it.

@dariusz-f dariusz-f dismissed their stale review May 21, 2025 12:58

Latest changes isn't working correctly

dkozinski added 2 commits May 21, 2025 18:36
Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>
…_HEADE

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>
Copy link
Collaborator

@dariusz-f dariusz-f left a comment

Choose a reason for hiding this comment

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

Now it's working correctly

#define __OAPV_CONFIG_H__9876543210123456789012345678901234567890123456789__

#define OAPV_LIBVERSION_HEADER 0 // 0 if the file oapv_libversion.h does not exist, otherwise 1
#define OAPV_EXPORT_HEADER 0 // 0 if the file oapv_exports.h does not exist, otherwise 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not correct, for both of those.
The checks just check for defined(), so even if defined to 0, that'll still succeed and try to include the headers.

…PORT_HEADER are defined, allowing inclusion of headers when defined as 0

Signed-off-by: Dawid Kozinski <d.kozinski@samsung.com>
@dariusz-f dariusz-f merged commit 00ade59 into AcademySoftwareFoundation:main May 23, 2025
5 checks passed
@jamrial jamrial mentioned this pull request May 28, 2025
@dariusz-f dariusz-f mentioned this pull request May 29, 2025
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

Successfully merging this pull request may close these issues.

5 participants