Fix FTBFS when tdehw is not present. This resolves bug 2494. #10

Merged
MicheleC merged 1 commits from fix/build-without-tdehw into master 3 months ago
Owner

When tdehw lib is not detected, all the preprocessor blocks defined by "TDE_HAVE_TDEHWLIB" will be skipped during the smoke file generation.

Tested successfully both when tdehw is present and not present.

When tdehw lib is not detected, all the preprocessor blocks defined by "TDE_HAVE_TDEHWLIB" will be skipped during the smoke file generation. Tested successfully both when tdehw is present and not present.
MicheleC added 1 commit 4 months ago
1bdb319ef5
Fix FTBFS when tdehw is not present. This resolves bug 2494.
MicheleC requested review from SlavekB 4 months ago
MicheleC added this to the R14.0.13 release milestone 4 months ago
SlavekB reviewed 4 months ago
SlavekB left a comment
Owner

This is a good idea, but there could be a cleaner solution than the dynamic patching of the script in the source directory – see comments below.

This is a good idea, but there could be a cleaner solution than the dynamic patching of the script in the source directory – see comments below.
configure.in.bot Outdated
echo ""
echo "tdehw lib was not found. Classes and methods from tdehw will not be included."
echo ""
perl -pe '$_ = qq($_\t\t\t\t\ \ \ \ \ \$p =~ m/^#\\s*ifdef\\s+__TDE_HAVE_TDEHWLIB/ or\n) if $_ =~ "Q_NO_USING_KEYWORD"' -i kalyptus/kalyptus
Owner

This is a good way, but a little dirty solution.

There is a problem that it dynamically changing the file in the source directory, which can represent a prohibited step for some strict distributions. It is true that tdebindings does not use building outside the source directory, so it may not be a major problem at this time, but it is a question whether to keep it or trying about a cleaner solution?

This is a good way, but a little _dirty_ solution. There is a problem that it dynamically changing the file in the source directory, which can represent a prohibited step for some strict distributions. It is true that tdebindings does not use building outside the source directory, so it may not be a major problem at this time, but it is a question whether to keep it or trying about a cleaner solution?
MicheleC marked this conversation as resolved
configure.in.in Outdated
CXXFLAGS="$CXXFLAGS $KDE_INCLUDES $TQT_INCLUDES $all_includes"
AC_TRY_COMPILE([
#include <tdehardwaredevices.h>
Owner

Maybe it could be a simpler test – verify if kdemacros.h defines __TDE_HAVE_TDEHWLIB?

Alternatively, such a test could be performed directly in the kalyptus script. And add __TDE_HAVE_TDEHWLIB to the removed preprocessor macros according to the test result dynamically directly in the kalyptus script. This could eliminate the need for dynamic script patching. What do you think about such a solution?

Maybe it could be a simpler test – verify if `kdemacros.h` defines `__TDE_HAVE_TDEHWLIB`? Alternatively, such a test could be performed directly in the kalyptus script. And add `__TDE_HAVE_TDEHWLIB` to the removed preprocessor macros according to the test result dynamically directly in the kalyptus script. This could eliminate the need for dynamic script patching. What do you think about such a solution?
Poster
Owner

Nice, I like this idea :-)

Nice, I like this idea :-)
MicheleC marked this conversation as resolved
MicheleC force-pushed fix/build-without-tdehw from 1bdb319ef5 to 2f653946e2 4 months ago
MicheleC force-pushed fix/build-without-tdehw from 2f653946e2 to 74afa30cd5 4 months ago
Poster
Owner

I ended up just checking for the existance of tdehardwaredevices.h directly in kalyptus. Much smaller change than the initial version :-)

I ended up just checking for the existance of tdehardwaredevices.h directly in kalyptus. Much smaller change than the initial version :-)
SlavekB requested changes 3 months ago
SlavekB left a comment
Owner

It does not work properly on a clean chroot – see the comment below.

It does not work properly on a clean chroot – see the comment below.
if (-e "$ENV{TDEDIR}/include/tdehardwaredevices.h")
{
$have_tdehw_lib = 1;
print "tdehw headers found\n";
Owner

Here it would be good to add the prefix "kalyptus: ", so that the build log shows where the report belongs.

Here it would be good to add the prefix "kalyptus: ", so that the build log shows where the report belongs.
MicheleC marked this conversation as resolved
else
{
$have_tdehw_lib = 0;
print "tdehw headers not found\n";
Owner

Here it would be good to add the prefix "kalyptus:", but also information where the header was searched for to help to analyze the problem. Something like:

print "kalyptus: tdehw headers not found at $ENV{TDEDIR}/include/tdehardwaredevices.h\n";

In any case, there is a problem, because when build on a clean chroot, there is not defined TDEDIR and the test is falsely negative.

Here it would be good to add the prefix "kalyptus:", but also information where the header was searched for to help to analyze the problem. Something like: ``` print "kalyptus: tdehw headers not found at $ENV{TDEDIR}/include/tdehardwaredevices.h\n"; ``` In any case, there is a problem, because when build on a clean chroot, there is not defined `TDEDIR` and the test is falsely negative.
MicheleC marked this conversation as resolved
SlavekB force-pushed fix/build-without-tdehw from 74afa30cd5 to 9e6f7cc6ed 3 months ago
Poster
Owner

New commit tested in clean chroot environment with and without tdehw lib. Works fine.

New commit tested in clean chroot environment with and without tdehw lib. Works fine.
MicheleC requested review from SlavekB 3 months ago
SlavekB approved these changes 3 months ago
SlavekB left a comment
Owner

It looks good.

It looks good.
MicheleC merged commit 9e6f7cc6ed into master 3 months ago
MicheleC deleted branch fix/build-without-tdehw 3 months ago

Reviewers

SlavekB approved these changes 3 months ago
The pull request has been merged as 9e6f7cc6ed.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.