Update cmake files for distros packaging

yarda commented 2 years ago

Signed-off-by: Jaroslav Škarvada jskarvad@redhat.com

yarda commented 2 years ago

This is copy of the original openfec upstream PR: https://github.com/OpenFEC/OpenFEC/pull/2

yarda commented 2 years ago

We are trying to get this package into Fedora, Fedora review request: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=2121558

gavv commented 1 year ago

Hi, sorry for delay, thanks for PR and detailed explanations.

After merging other PRs, this one got conflicts. I've rebased it on fresh main and force-pushed to distro-cmake.

When rebasing, I've removed one change:

set(CMAKE_C_FLAGS "-O4")

IIRC, using O3 was important in case of OpenFEC (and O4 = O3). Unfortunately I don't remember exact details.

This should be set by host/distro. In case it's really needed, it should have CACHE keyword to be overridable by distro CFLAGS.

I see. I'll address it in separate commit, so that by default it will be O3, but could be overwritten. That is what is needed for packaging, right? But I recommend to package this lib with O3.

gavv commented 1 year ago

I've pushed follow-up commit: Add OPTIMIZE option (65bd0cbc53e3d3ad6bb6884a4fe45ec4f541f547)

By default, build with -O3
If user specifies -DOPTIMIZE=<N>, build with -O<N>
If user specifies -DOPTIMIZE=DEFAULT, don't touch -O

This way, we preserve behavior of forcing -O3 by default, but add a way to override it.

Please let me know if that suits your need or we need some other changes.

gavv commented 1 year ago

Tagged v1.4.2.5 with this fix included.

yarda commented 1 year ago

I've pushed follow-up commit: Add OPTIMIZE option (65bd0cb)
* By default, build with -O3

* If user specifies `-DOPTIMIZE=<N>`, build with `-O<N>`

* If user specifies `-DOPTIMIZE=DEFAULT`, don't touch `-O`
This way, we preserve behavior of forcing -O3 by default, but add a way to override it.

Please let me know if that suits your need or we need some other changes.

Thanks, LGTM.

gavv commented 1 year ago

@yarda Could you clarify why do we need install targets for executables? (eperftool, simple_server, simple_client, descr_stats)

In my understanding, they all are either internal developer tools or examples, not intended for end users.

Can we remove these install targets?

gavv commented 1 year ago

@yarda Could you also explain what's wrong with EXECUTABLE_OUTPUT_PATH?

I mean this set of changes:

- add_test("create_instance" ${EXECUTABLE_OUTPUT_PATH}/test_create_instance)
+ add_test("create_instance" ${PROJECT_BINARY_DIR}/tests/test_create_instance)

This changes broke openfec tests. I'm now trying to find the best way to fix them.

yarda commented 1 year ago

@yarda Could you clarify why do we need install targets for executables? (eperftool, simple_server, simple_client, descr_stats)

In my understanding, they all are either internal developer tools or examples, not intended for end users.

Can we remove these install targets?

We usually package everything that's available in the upstream project including internal tools/tools for developers, because the Fedora package can also be (and usually is) used by developers. These tools are currently packaged in the openfec-utils (I was also thinking about the openfec-tools name) sub package.

If you think these tools are not needed, maybe they could be removed from the release tarball (and still kept in the git or removed from the git if not useful at all).

Or if you think these tools can be useful for developers and shouldn't be installed by the end users, maybe you could provide specific target for them? I.e. not to install them by default.

I think the current state when it is compiled by default and not installed is not optimal.

yarda commented 1 year ago

@yarda Could you also explain what's wrong with EXECUTABLE_OUTPUT_PATH?

I mean this set of changes:
- add_test("create_instance" ${EXECUTABLE_OUTPUT_PATH}/test_create_instance)
+ add_test("create_instance" ${PROJECT_BINARY_DIR}/tests/test_create_instance)
This changes broke openfec tests. I'm now trying to find the best way to fix them.

The build dir (the directory holding the built binaries) can be (and on Fedora is) elsewhere (out of the tree build). As I understand these cmake variables ${PROJECT_BINARY_DIR} is full path to the project build directory, i.e. path for anything that is built. The ${EXECUTABLE_OUTPUT_PATH} expands to the ${PROJECT_SOURCE_DIR}/perf_eval and the ${PROJECT_SOURCE_DIR} is the path where the sources are unpacked.

gavv commented 1 year ago

Thanks,

We usually package everything that's available in the upstream project including internal tools/tools for developers, because the Fedora package can also be (and usually is) used by developers. These tools are currently packaged in the openfec-utils (I was also thinking about the openfec-tools name) sub package.

If you think these tools are not needed, maybe they could be removed from the release tarball (and still kept in the git or removed from the git if not useful at all).

Personally I think that:

if I'm not developing openfec, I don't need them
if I'm developing openefec, I need they built from my source tree, not from distro; because they should reflect modifications I made

On the other hand, if the convention is to package such stuff, then OK. It's just a bit surprising to me, but I'm not very familiar with packaging conventions.

Or if you think these tools can be useful for developers and shouldn't be installed by the end users, maybe you could provide specific target for them? I.e. not to install them by default.

Sounds good. I'll add separate target, so that package maintainers can decide whether to use it or not.

The build dir (the directory holding the built binaries) can be (and on Fedora is) elsewhere (out of the tree build). As I understand these cmake variables [...]

I see...

When you're building openfec in-tree, binaries are by default installed into EXECUTABLE_OUTPUT_PATH, which is by default set to ${PROJECT_SOURCE_DIR}/bin/${CMAKE_BUILD_TYPE}, e.g. bin/Release.

Then, when you try to run tests, tests expect to find those tools in EXECUTABLE_OUTPUT_PATH.

Note that PROJECT_BINARY_DIR and EXECUTABLE_OUTPUT_PATH are different. Usually, PROJECT_BINARY_DIR is build, and EXECUTABLE_OUTPUT_PATH is bin/Release. And build contains objects files, while bin/Release contains executables.

How exactly does Fedora run cmake?

Does it set EXECUTABLE_OUTPUT_PATH? Or CMAKE_RUNTIME_OUTPUT_DIRECTORY?
Does it set LIBRARY_OUTPUT_PATH?

One possible solution is:

don't use neither PROJECT_SOURCE_DIR nor PROJECT_BINARY_DIR for executables
instead, create executables inside EXECUTABLE_OUTPUT_PATH
by default set EXECUTABLE_OUTPUT_PATH to bin/Release
when creating package for Fedora, overwrite EXECUTABLE_OUTPUT_PATH with whatever you need, e.g. with the same value as PROJECT_BINARY_DIR

What do you think?

gavv commented 1 year ago

I've implemented the approach that I described above in this PR: #14.

@yarda could you take a look?

yarda commented 1 year ago

Thanks,

We usually package everything that's available in the upstream project including internal tools/tools for developers, because the Fedora package can also be (and usually is) used by developers. These tools are currently packaged in the openfec-utils (I was also thinking about the openfec-tools name) sub package.

If you think these tools are not needed, maybe they could be removed from the release tarball (and still kept in the git or removed from the git if not useful at all).

Personally I think that:
* if I'm not developing openfec, I don't need them

* if I'm developing openefec, I need they built from my source tree, not from distro; because they should reflect modifications I made
On the other hand, if the convention is to package such stuff, then OK. It's just a bit surprising to me, but I'm not very familiar with packaging conventions.

Or if you think these tools can be useful for developers and shouldn't be installed by the end users, maybe you could provide specific target for them? I.e. not to install them by default.

Sounds good. I'll add separate target, so that package maintainers can decide whether to use it or not.

The build dir (the directory holding the built binaries) can be (and on Fedora is) elsewhere (out of the tree build). As I understand these cmake variables [...]

I see...

When you're building openfec in-tree, binaries are by default installed into EXECUTABLE_OUTPUT_PATH, which is by default set to ${PROJECT_SOURCE_DIR}/bin/${CMAKE_BUILD_TYPE}, e.g. bin/Release.

Then, when you try to run tests, tests expect to find those tools in EXECUTABLE_OUTPUT_PATH.

Note that PROJECT_BINARY_DIR and EXECUTABLE_OUTPUT_PATH are different. Usually, PROJECT_BINARY_DIR is build, and EXECUTABLE_OUTPUT_PATH is bin/Release. And build contains objects files, while bin/Release contains executables.

How exactly does Fedora run cmake?
* Does it set `EXECUTABLE_OUTPUT_PATH`? Or `CMAKE_RUNTIME_OUTPUT_DIRECTORY`?

* Does it set `LIBRARY_OUTPUT_PATH`?
One possible solution is:
* don't use neither `PROJECT_SOURCE_DIR` nor `PROJECT_BINARY_DIR` for executables

* instead, create executables inside `EXECUTABLE_OUTPUT_PATH`

* by default set `EXECUTABLE_OUTPUT_PATH` to `bin/Release`

* when creating package for Fedora, overwrite `EXECUTABLE_OUTPUT_PATH` with whatever you need, e.g. with the same value as `PROJECT_BINARY_DIR`
What do you think?

In fedora cmake is run with the following arguments:

 /usr/bin/cmake \
        -S "." \
        -B "redhat-linux-build" \
        -DCMAKE_C_FLAGS_RELEASE:STRING="-DNDEBUG" \
        -DCMAKE_CXX_FLAGS_RELEASE:STRING="-DNDEBUG" \
        -DCMAKE_Fortran_FLAGS_RELEASE:STRING="-DNDEBUG" \
        -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON \
        -DCMAKE_INSTALL_DO_STRIP:BOOL=OFF \
        -DCMAKE_INSTALL_PREFIX:PATH=/usr \
        -DINCLUDE_INSTALL_DIR:PATH=/usr/include \
        -DLIB_INSTALL_DIR:PATH=/usr/lib64 \
        -DSYSCONF_INSTALL_DIR:PATH=/etc \
        -DSHARE_INSTALL_PREFIX:PATH=/usr/share \
%if "lib64" == "lib64" 
        -DLIB_SUFFIX=64 \
%endif 
        -DBUILD_SHARED_LIBS:BOOL=ON

/usr/bin/cmake --build "redhat-linux-build" -j3 --verbose

DESTDIR="/home/yarda/rpmbuild/BUILDROOT/%{NAME}-%{VERSION}-%{RELEASE}.x86_64" /usr/bin/cmake --install "redhat-linux-build"

It works with most of the cmake projects, but IIRC I had problems with the original EXECUTABLE_OUTPUT_PATH. I will retry with the current version.

yarda commented 1 year ago

I've implemented the approach that I described above in this PR: #14.

@yarda could you take a look?

I will check it.

gavv commented 1 year ago

In fedora cmake is run with the following arguments: [...]

redhat-linux-build is a sub-directory inside source dir, right?

Are there any requirements where the executables and shared objects should be located after invoking --build and before invoking --install?

Is it wrong if they are located in bin/Release in source dir?

gavv commented 1 year ago

cmake -DINSTALL_DEVTOOLS=ON \                         
        -S "." \  
        -B "redhat-linux-build" \
        -DCMAKE_C_FLAGS_RELEASE:STRING="-DNDEBUG" \
        -DCMAKE_CXX_FLAGS_RELEASE:STRING="-DNDEBUG" \
        -DCMAKE_Fortran_FLAGS_RELEASE:STRING="-DNDEBUG" \
        -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON \
        -DCMAKE_INSTALL_DO_STRIP:BOOL=OFF \
        -DCMAKE_INSTALL_PREFIX:PATH=/usr \
        -DINCLUDE_INSTALL_DIR:PATH=/usr/include \
        -DLIB_INSTALL_DIR:PATH=/usr/lib64 \
        -DSYSCONF_INSTALL_DIR:PATH=/etc \
        -DSHARE_INSTALL_PREFIX:PATH=/usr/share \
        -DLIB_SUFFIX=64 \
        -DBUILD_SHARED_LIBS:BOOL=ON

make -C redhat-linux-build

DESTDIR=/tmp/test cmake --install "redhat-linux-build"

find /tmp/test -maxdepth 5

/tmp/test
/tmp/test/usr
/tmp/test/usr/include
/tmp/test/usr/include/openfec
/tmp/test/usr/include/openfec/lib_common
/tmp/test/usr/include/openfec/lib_common/of_mem.h
/tmp/test/usr/include/openfec/lib_common/statistics
/tmp/test/usr/include/openfec/lib_common/of_openfec_profile.h
/tmp/test/usr/include/openfec/lib_common/of_types.h
/tmp/test/usr/include/openfec/lib_common/of_openfec_api.h
/tmp/test/usr/include/openfec/lib_common/of_rand.h
/tmp/test/usr/include/openfec/lib_common/of_debug.h
/tmp/test/usr/include/openfec/lib_common/linear_binary_codes_utils
/tmp/test/usr/include/openfec/lib_common/of_cb.h
/tmp/test/usr/include/openfec/lib_advanced
/tmp/test/usr/include/openfec/lib_advanced/ldpc_from_file
/tmp/test/usr/include/openfec/lib_stable
/tmp/test/usr/include/openfec/lib_stable/reed-solomon_gf_2_8
/tmp/test/usr/include/openfec/lib_stable/2d_parity_matrix
/tmp/test/usr/include/openfec/lib_stable/ldpc_staircase
/tmp/test/usr/include/openfec/lib_stable/reed-solomon_gf_2_m
/tmp/test/usr/share
/tmp/test/usr/share/pkgconfig
/tmp/test/usr/share/pkgconfig/openfec.pc
/tmp/test/usr/bin
/tmp/test/usr/bin/eperftool
/tmp/test/usr/bin/simple_server
/tmp/test/usr/bin/simple_client
/tmp/test/usr/lib
/tmp/test/usr/lib/x86_64-linux-gnu
/tmp/test/usr/lib/x86_64-linux-gnu/libopenfec.so.1.4.2
/tmp/test/usr/lib/x86_64-linux-gnu/libopenfec.so.1
/tmp/test/usr/lib/x86_64-linux-gnu/libopenfec.so

find bin

bin
bin/Release
bin/Release/libopenfec.so.1.4.2
bin/Release/test_create_instance
bin/Release/eperftool
bin/Release/test_code_params
bin/Release/libopenfec.so
bin/Release/libopenfec.so.1
bin/Release/simple_client
bin/Release/simple_server
bin/Release/test_encoder_instance

gavv commented 1 year ago

@yarda Hi, I've merged this PR because it's needed for package for arch. But let me know if you'll have any issues or comments.

yarda commented 1 year ago

Sorry for the delay. Now I am trying to rebase to openfec-1.4.2.5, but I have still problems with the tests:

+ cd openfec-1.4.2.5
+ cd redhat-linux-build
+ make test
Running tests...
/usr/bin/ctest --force-new-ctest-process.
Test project /builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build
        Start   1: create_instance
Could not find executable /builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/test_create_instance
Looked in the following places:
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Release/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Release/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Debug/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Debug/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/MinSizeRel/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/MinSizeRel/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/RelWithDebInfo/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/RelWithDebInfo/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Deployment/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Deployment/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Development/test_create_instance
/builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Development/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Release/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Release/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Debug/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Debug/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/MinSizeRel/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/MinSizeRel/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/RelWithDebInfo/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/RelWithDebInfo/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Deployment/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Deployment/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Development/test_create_instance
builddir/build/BUILD/openfec-1.4.2.5/redhat-linux-build/tests/Development/test_create_instance
  1/265 Test   #1: create_instance ..................***Not Run   0.00 sec

Full build log: https://kojipkgs.fedoraproject.org//work/tasks/8169/96708169/build.log

Moreover I spotted two new problems, problem 1:

*** WARNING: ./usr/src/debug/openfec-1.4.2.5-1.fc38.x86_64/src/lib_common/linear_binary_codes_utils/binary_matrix/of_tools.c is executable but has no shebang, removing executable bit
*** WARNING: ./usr/src/debug/openfec-1.4.2.5-1.fc38.x86_64/src/lib_common/linear_binary_codes_utils/binary_matrix/of_tools.h is executable but has no shebang, removing executable bit
*** WARNING: ./usr/src/debug/openfec-1.4.2.5-1.fc38.x86_64/src/lib_stable/reed-solomon_gf_2_8/of_reed-solomon_gf_2_8_api.h is executable but has no shebang, removing executable bit
*** WARNING: ./usr/src/debug/openfec-1.4.2.5-1.fc38.x86_64/src/lib_stable/reed-solomon_gf_2_8/of_reed-solomon_gf_2_8.c is executable but has no shebang, removing executable bit
*** WARNING: ./usr/src/debug/openfec-1.4.2.5-1.fc38.x86_64/applis/eperftool/blocking_struct.c is executable but has no shebang, removing executable bit
*** WARNING: ./usr/src/debug/openfec-1.4.2.5-1.fc38.x86_64/applis/eperftool/blocking_struct.h is executable but has no shebang, removing executable bit
*** WARNING: ./usr/src/debug/openfec-1.4.2.5-1.fc38.x86_64/applis/howto_examples/simple_client_server/simple_server.c is executable but has no shebang, removing executable bit
*** WARNING: ./usr/src/debug/openfec-1.4.2.5-1.fc38.x86_64/applis/howto_examples/simple_client_server/simple_client.c is executable but has no shebang, removing executable bit
*** WARNING: ./usr/src/debug/openfec-1.4.2.5-1.fc38.x86_64/applis/howto_examples/simple_client_server/simple_client_server.h is executable but has no shebang, removing executable bit
*** WARNING: ./usr/src/debug/openfec-1.4.2.5-1.fc38.x86_64/tools/descr_stats_v1.2/descr_stats.c is executable but has no shebang, removing executable bit

I.e. the source files shouldn't have the executable bit in the released tarball.

Problem 2:

-- Detecting C compile features
-- Detecting C compile features - done
CMake Error at CMakeLists.txt:12 (string):
  string sub-command REGEX, mode MATCH needs at least 5 arguments total to
  command.
-- Detected version from git:.
-- Debug mode OFF
-- Optimization level DEFAULT
-- Configuring "/openfec.pc"
-- Configuring incomplete, errors occurred!

It's because it's running git describe on the line 10 of the CMakeLists.txt and there are no git metadata in the released tarball. I.e. the version detection needs fallback by e.g. grepping some header file, etc.

gavv commented 1 year ago

No worries for delay. For the git describe problem, could you try 1.4.2.6 instead? It should be fixed. I'll take a look at the other stuff..

yarda commented 1 year ago

With the 1.4.2.6 the git describe problem is gone and the tests also work. There is still the following:

*** WARNING: ./usr/src/debug/openfec-1.4.2.6-1.fc38.x86_64/src/lib_common/linear_binary_codes_utils/binary_matrix/of_tools.c is executable but has no shebang, removing executable bit
*** WARNING: ./usr/src/debug/openfec-1.4.2.6-1.fc38.x86_64/src/lib_common/linear_binary_codes_utils/binary_matrix/of_tools.h is executable but has no shebang, removing executable bit
*** WARNING: ./usr/src/debug/openfec-1.4.2.6-1.fc38.x86_64/src/lib_stable/reed-solomon_gf_2_8/of_reed-solomon_gf_2_8_api.h is executable but has no shebang, removing executable bit
*** WARNING: ./usr/src/debug/openfec-1.4.2.6-1.fc38.x86_64/src/lib_stable/reed-solomon_gf_2_8/of_reed-solomon_gf_2_8.c is executable but has no shebang, removing executable bit

I.e. 4 files have wrong mode in the tarball. Otherwise LGTM.

gavv commented 1 year ago

@yarda I've tagged 1.4.2.8, which removes those executable bits. Seems they come from original source tarball.

https://github.com/roc-streaming/openfec/releases/tag/v1.4.2.8

roc-streaming / openfec

Update cmake files for distros packaging #6