TDE
/
kdbusnotification
6
0
Fork 0
Code Issues Pull Requests 1 Releases Wiki Activity
Labels Milestones

Respect -DDBUS_BINDING_TOOL provided by cmake user #7

Merged
MicheleC merged 1 commits from fiat/respect-tool into master 5 months ago
Conversation 7 Commits 1 Files Changed 2
Fat-Zer commented 5 months ago
Collaborator

A user might want to specify full path to dbus-binding-tool. We should respect that.
This might be useful in case of cross-compiling.

A user might want to specify full path to `dbus-binding-tool`. We should respect that. This might be useful in case of cross-compiling.
Fat-Zer added 1 commit 5 months ago
MicheleC reviewed 5 months ago
ConfigureChecks.cmake
if( "${DBUS_BINDING_TOOL}" STREQUAL "DBUS_BINDING_TOOL-NOTFOUND" )
MicheleC commented 5 months ago
Owner

Shouldn't we test that DBUS_BINDING_TOOL is empty before we try to detect it? If a user passes a custom path using -DDBUS_BINDING_TOOL=..., then there should be no need to look for the program. Perhap we could add a check that the file actually exists in that case. What do you think?

Shouldn't we test that `DBUS_BINDING_TOOL` is empty before we try to detect it? If a user passes a custom path using `-DDBUS_BINDING_TOOL=...`, then there should be no need to look for the program. Perhap we could add a check that the file actually exists in that case. What do you think?
Fat-Zer commented 5 months ago
Poster
Collaborator

Nah... find_program() uses CACHE variables by default it won't look for the program if the variable already defined...

Nah... `find_program()` uses `CACHE` variables by default it won't look for the program if the variable already defined...
MicheleC marked this conversation as resolved
ormorph commented 5 months ago
Collaborator

This solution doesn't work at all. In any case, dbus-binding-tool is used. If the variable is set, then the check simply passes, but the dbus-binding-tool command is used. This command can be renamed, but it cannot be assigned, since dbus-binding-tool will still be used. You will get the following error output:

[1/5] cd /var/tmp/portage/trinity-apps/kdbusnotification-9999/work/kdbusnotification-9999_build/src/daemon && dbus-binding-tool --mode=glib-server --prefix=notification_daemon_tde /var/tmp/portage/trinity-apps/kdbusnotification-9999/work/kdbusnotification-9999/src/daemon/notificationdaemon.xml > notificationdaemon-dbus-glue.h
FAILED: src/daemon/notificationdaemon-dbus-glue.h /var/tmp/portage/trinity-apps/kdbusnotification-9999/work/kdbusnotification-9999_build/src/daemon/notificationdaemon-dbus-glue.h 
cd /var/tmp/portage/trinity-apps/kdbusnotification-9999/work/kdbusnotification-9999_build/src/daemon && dbus-binding-tool --mode=glib-server --prefix=notification_daemon_tde /var/tmp/portage/trinity-apps/kdbusnotification-9999/work/kdbusnotification-9999/src/daemon/notificationdaemon.xml > notificationdaemon-dbus-glue.h
bin/sh: line 1: dbus-binding-tool: command not found
This solution doesn't work at all. In any case, dbus-binding-tool is used. If the variable is set, then the check simply passes, but the `dbus-binding-tool` command is used. This command can be renamed, but it cannot be assigned, since `dbus-binding-tool` will still be used. You will get the following error output: ``` [1/5] cd /var/tmp/portage/trinity-apps/kdbusnotification-9999/work/kdbusnotification-9999_build/src/daemon && dbus-binding-tool --mode=glib-server --prefix=notification_daemon_tde /var/tmp/portage/trinity-apps/kdbusnotification-9999/work/kdbusnotification-9999/src/daemon/notificationdaemon.xml > notificationdaemon-dbus-glue.h FAILED: src/daemon/notificationdaemon-dbus-glue.h /var/tmp/portage/trinity-apps/kdbusnotification-9999/work/kdbusnotification-9999_build/src/daemon/notificationdaemon-dbus-glue.h cd /var/tmp/portage/trinity-apps/kdbusnotification-9999/work/kdbusnotification-9999_build/src/daemon && dbus-binding-tool --mode=glib-server --prefix=notification_daemon_tde /var/tmp/portage/trinity-apps/kdbusnotification-9999/work/kdbusnotification-9999/src/daemon/notificationdaemon.xml > notificationdaemon-dbus-glue.h bin/sh: line 1: dbus-binding-tool: command not found ```
Fat-Zer commented 5 months ago
Poster
Collaborator

@ormorph, I believe you are doing something wrong: either using a branch without the patch or supplying some bogus arguments to cmake... Could you add the full build.log and your exact actions (and may be environment)?

@ormorph, I believe you are doing something wrong: either using a branch without the patch or supplying some bogus arguments to cmake... Could you add the full `build.log` and your exact actions (and may be `environment`)?
ormorph commented 5 months ago
Collaborator

Yes, there is such a thing, I got carried away, at first I did everything right:

# EGIT_OVERRIDE_BRANCH_GITEA_TDE_KDBUSNOTIFICATION=fiat/respect-tool emerge -av1 kdbusnotification

Then I got carried away and did this:

emerge -av1 kdbusnotification

In principle it works, you just need to check all the options.

Yes, there is such a thing, I got carried away, at first I did everything right: ``` # EGIT_OVERRIDE_BRANCH_GITEA_TDE_KDBUSNOTIFICATION=fiat/respect-tool emerge -av1 kdbusnotification ``` Then I got carried away and did this: ``` emerge -av1 kdbusnotification ``` In principle it works, you just need to check all the options.
ormorph commented 5 months ago
Collaborator

There is one drawback here: if the -DDBUS_BINDING_TOOL parameter is specified, then the check for the presence of the file does not pass. Only an error during build reports a problem.

There is one drawback here: if the `-DDBUS_BINDING_TOOL` parameter is specified, then the check for the presence of the file does not pass. Only an error during build reports a problem.
Fat-Zer commented 5 months ago
Poster
Collaborator

There is one drawback here: if the -DDBUS_BINDING_TOOL parameter is specified, then the check for the presence of the file does not pass. Only an error during build reports a problem.

Not much of a drawback per se: any bogus value before the patch would result in pretty much the same behavior: dbus-binding-tool would be called wihout any checks as you could notice...

We neither check in tdebase if DBUSXML2QT3_EXECUTABLE from dbus-1-tqt is correct ... IMHO Those variables are quite niche and the tools are not as significant as e.g. compilers to justify hassling with such checks...

> There is one drawback here: if the -DDBUS_BINDING_TOOL parameter is specified, then the check for the presence of the file does not pass. Only an error during build reports a problem. Not much of a drawback per se: any bogus value before the patch would result in pretty much the same behavior: `dbus-binding-tool` would be called wihout any checks as you could notice... We neither check in tdebase if `DBUSXML2QT3_EXECUTABLE` from dbus-1-tqt is correct ... IMHO Those variables are quite niche and the tools are not as significant as e.g. compilers to justify hassling with such checks...
ormorph commented 5 months ago
Collaborator

IMHO Those variables are quite niche and the tools are not as significant as e.g. compilers to justify hassling with such checks...

To be honest, I don’t really understand the convenience of this. If I need to change the path and program, then I will do it by correcting the config. For the average user, the -DDBUS_BINDING_TOOL parameter does not say anything, nor how to use it, since it is not visible in CMakeLists.txt.

> IMHO Those variables are quite niche and the tools are not as significant as e.g. compilers to justify hassling with such checks... To be honest, I don’t really understand the convenience of this. If I need to change the path and program, then I will do it by correcting the config. For the average user, the `-DDBUS_BINDING_TOOL` parameter does not say anything, nor how to use it, since it is not visible in `CMakeLists.txt`.
MicheleC approved these changes 5 months ago
MicheleC left a comment
Owner

Looks good

Looks good
MicheleC merged commit da111a6195 into master 5 months ago
MicheleC deleted branch fiat/respect-tool 5 months ago
MicheleC added this to the R14.1.2 release milestone 5 months ago
MicheleC commented 5 months ago
Owner

Thanks Alex!

Thanks Alex!

Reviewers

MicheleC approved these changes 5 months ago
The pull request has been merged as da111a6195.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
MicheleC
Labels
Apply labels
Clear labels
GE/need-info
General - need additional info from contributor PR/keep-branch
Pull request - do not delete branch after merging PR/not-ok
Pull request - need fixing PR/rfc
Pull request - request for comments PR/update-trans
Pull request - update to translation files needed PR/wip
Pull request - work in progress RS/R14.0.x
Related to R14.0.x series RS/R14.1.x
Related to R14.1.x series SL/critical
Severity level - critical SL/major
Severity level - major SL/minor
Severity level - minor SL/normal
Severity level - normal SL/regression
Severity level - regression from previous version SL/trivial
Severity level - trivial SL/wishlist
Severity level - wishlist request ST/duplicate
Status - duplicate of another issue ST/invalid
Status - invalid report ST/notourproblem
Status - not our problem ST/rejected
Status - rejected ST/wontfix
Status - won't fix ST/worksforme
Status - works for me, unable to reproduce
No Label GE/need-info PR/keep-branch PR/not-ok PR/rfc PR/update-trans PR/wip RS/R14.0.x RS/R14.1.x SL/critical SL/major SL/minor SL/normal SL/regression SL/trivial SL/wishlist ST/duplicate ST/invalid ST/notourproblem ST/rejected ST/wontfix ST/worksforme
Milestone
Set milestone
Clear milestone
No items
No Milestone
R14.1.2 release
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/kdbusnotification#7
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'fiat/respect-tool'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes