Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debian package fix lintian errors #437

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

yurtpage
Copy link

@yurtpage yurtpage commented Jul 27, 2024

@kvvloten please review.

The set cmake_minimum_required to 3.5 I made to fix a deprecation warning.

Now we have two ways to build a deb package: the standard debuild and the make package to build the deb package directly from the cmake with CPack. I found this confusing and unless the CPack is really used I would propose to remove it.

UPD fixed in 468dcc8 Another problem is that tests are included by default unless the ADMC_BUILD_DEB var is not set. Because of this we have to specify the ADMC_BUILD_DEB in the debian/rules file.
Maybe the test should be implemented with the CMake add_test or something like that to make it more standard. The the debhelper will just skip the tests in the convenient way.

yurtpage added 4 commits July 28, 2024 07:44
The CMake has a built-in variable BUILD_TESTING (default ON) to compile tests.

When building a debian package, the debhelper configures the CMake with this option OFF.
So now we don't need to pass the DADMC_BUILD_DEB=ON and we can simplify the debian/rules
Copy link
Collaborator

@august-alt august-alt Aug 6, 2024

Choose a reason for hiding this comment

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

What is the purpose of modifying the README.md file in a pull request that specifically addresses Debian packaging? The changes to README.md are unrelated to the purpose of this pull request, please consider removing them and submitting a separate pull request for those changes if you still wish to update the file."

@@ -52,9 +52,9 @@ include(ClangFormat)
option(ADMC_BUILD_DEB "Build the deb package of ADMC." OFF)

add_subdirectory(src)
if(NOT ADMC_BUILD_DEB)
if(BUILD_TESTING)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I recall correctly, the BUILD_TESTING variable is set up by CTest, which is not currently enabled in this project (see
https://cmake.org/cmake/help/latest/module/CTest.html
). Instead, we use the ADMC_BUILD_DEB variable, which is OFF by default. We need to consider one of three options: creating and setting a separate variable to explicitly enable or disable testing, setting ADMC_BUILD_DEB to ON by default, or completely refactoring the test setup.

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.

4 participants