-
Notifications
You must be signed in to change notification settings - Fork 123
Conversation
src/plugins/xfconf/xfconf.c
Outdated
GError * err = NULL; | ||
if (xfconf_init (&err)) | ||
{ | ||
ELEKTRA_LOG_DEBUG ("succeed initielize xfconf\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initialize
# Backup-and-Restore: user:/tests/xfconf | ||
|
||
# mount the xfwm channel | ||
kdb mount /dev/null /test/xfwm xfconf "channel=xfwm4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plugin also needs new-backend and new-style mounting. Let us discuss this later in the meeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I realized it is the same issue as for @flo91 has with the relational database backend
5088828
to
d2af9e4
Compare
I ran into an issue regarding the macOS images: xfconf is only available for macOS through macports but not homebrew so I wonder what am I supposed to do. I see multiple options:
Another problem I ran into is regarding the FreeBSD images: all of them refuse linking the xfconf-plugin and I am not able to recognize the exact problem here. I have installed xfconf on the FreeBSD images. Before doing that the images threw a compilation error which tells me that the FreeBSD images are able to find the xfconf library at least kind of in some cases. Here is the error log regarding the linking process:
I would be glad about any suggestions. |
Sounds sensible. Using XFCE under macOS is probably a rare thing. Without actually testing if the macports xfce even works it would be a waste of (CPU) time doing regression checks.
Sounds like linker path is incomplete. Where are these files (xfconf-0*.so etc.)? |
That is right, so I will exclude it.
Using
prints
|
Weird, /usr/local/lib is in the linker path and ldconfig was executed? What does Is something in the CMake error logs? |
Nothing useful for this issue. Unfortunately, I am unable to build it on my local FreeBSD machine too because of:
despite |
08cf8d6
to
96bd0ec
Compare
I managed to reproduce the same error on my local FreeBSD machine. I let clang output the |
Please disable your plugin/binding if PkgConfig is not found (see e.g. src/bindings/gsettings/CMakeLists.txt). As you probably want to reuse the xfconf detection code, it also makes sense to create a xfconf module in scripts/cmake/Modules/. First check if something like this was already written by someone else, this might actually solve the whole problem we face, as this module might work perfectly with FreeBSD. If it does not help: There is probably some FreeBSD-specific solution I do not know. In two modules (scripts/cmake/Modules/FindLibJWT.cmake and FindMySqlCppConn.cmake ) we have /usr/local/lib in PATHS of find_library (this would be an alternative way not using pkgconfig). But as many other modules also seem to work for you, this might not be needed. I also would need a local FreeBSD setup to find out the differences of why also other modules link correctly (if they do?). |
Thank you for the tip. I created a cmake module for xfconf based on |
Now, the FreeBSD machines throw
|
We probably need build logs to find anything out about it. Imho it is a bug, it should be possible to build test cases which cannot be run afterwards. @kodebach Do you have an idea? |
Looks like the codegen-based examples cannot be built. The build logs also show
So I guess that's the issue. No idea why it would've worked before, but not now. |
Looks like gcc is hardcoded somewhere where actually the compiler detected by cmake should be used. |
This was in one of the |
In doc/TESTING.md we actually require gcc to be installed. This is probably only because of pythongen, which should be removed anyway. The proper C compiler is "CMAKE_C_COMPILER" (or CMAKE_CXX_COMPILER for C++), not cc, which we do not require to exist (see doc/TESTING.md). |
0edda3f
to
96ff190
Compare
96ff190
to
6f9278b
Compare
I still have this issue with freebsd and I compared the output from the ci log of this PR with the current master (this PR is ahead of master). The first difference I can find is:
May this be the root cause of the problem? If yes, does anybody know how to generate |
6f9278b
to
b20f230
Compare
I think this is a different problem. I wonder why this test is even executed in the installed Elektra?
But your approach sounds promising: which other differences are there? |
I think this is related #2523. The test in question is one of the ones in |
This is the complete output what this branch produces but not the master. Click to expand
and
But I am not able why this problem only exist on this branch and not on the master. I cannot see anything related to the xfconf plugin. The only problem seems to be the missing gcc command. |
I don't think this really can be related to
This doesn't answer why the job fails, but I'm pretty sure it is not related to xfconf. Interestingly, AFAICT Lines 49 to 62 in cf4b6f3
Maybe just adding it will fix this. It doesn't explain why it works on Even better would be to use the same compiler in the tests that we use to actually compile Elektra (so An easy way to keep the scripts also working as an example would be using |
caca683
to
390413e
Compare
@markus2330 finally, from my side it can be merged now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, clean and nice PR. Docu&Tests need some improvements, though.
|
||
## Examples | ||
|
||
```zsh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use (also) shell recorder for a few tests.
src/plugins/xfconf/README.md
Outdated
|
||
## Introduction | ||
|
||
This is a storage plugin to mount the xfconf configuration settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain what this plugin can do, how to use it, what works, ... E.g. see src/plugins/augeas/README.md for a similar plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a much better introduction with 56e94e4.
src/plugins/xfconf/README.md
Outdated
|
||
## Dependencies | ||
|
||
xfconf from the XFCE project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please tell which versions you tested, how the package might be called etc.
According to your changes, dbus also seem to be important here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have extended this section in the commit 0f5f854
## Limitations | ||
|
||
- usage of a dummy file such as `/dev/null` | ||
- xfconf locks can only be read but not set as this is not possible in xfconf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain what xfconf locks are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with b135305.
printf ("open plugin...\n"); | ||
PLUGIN_OPEN ("xfconf"); | ||
|
||
KeySet * ks = ksNew (0, KS_END); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are only very basic tests.
@@ -46,6 +46,10 @@ for PLUGIN in $ACTUAL_PLUGINS; do | |||
# output on open/close | |||
continue | |||
;; | |||
"xfconf") | |||
# dbus is not enable on internal checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# dbus is not enable on internal checks | |
# dbus is not enabled on internal checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with 49cd309.
- infos/provides = storage/xfconf | ||
- infos/recommends = | ||
- infos/placements = postgetstorage presetstorage | ||
- infos/status = maintained libc configurable experimental unfinished concept limited memleak |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why unfinished, what is missing? What do you mean with limited? The limitations below seem to be general limitations of xfconf. If you do everything that xfconf can do, you don't need to declare your plugin limited. Please clarify in the README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still open.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! Some clarifications in README.md still needed.
src/plugins/xfconf/README.md
Outdated
|
||
### Property | ||
|
||
A property in xfconf is the same as a key in libelektra i.e. it has a name and can hold one or more values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A key only has one value (or none). Can you please give an example of how a property in xfconf would be represented in Elektra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, this was may fault. I corrected this with d1e4892.
src/plugins/xfconf/README.md
Outdated
|
||
The list of all channels is stored in the `system:/elektra/modules/xfconf/channels` which is an array of all channel names. | ||
Channel cannot be explicitly created or removed. | ||
They only exist in an implicit manner when you pass the `channel=...` argument. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this sentence. Do you mean "plugin configuration" when you say argument? Please describe how channels are used within xfce4, and how they map to Elektra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope the commit e282a32 makes it more clear.
xfconf from the XFCE project | ||
The xfconf library from the XFCE project is the main dependency of this plugin. | ||
Usually, this library is called something such as `xfconf` (Arch, Fedora, `xfconf-devel` for compiling), `libxfconf-0` (Debian, `libxfconf-0-dev` for compmiling) or `xfce4-conf` (FreeBSD) in the package manager. | ||
As xfconf itself depends on dbus and glib, these are dependecies too but should be installed with the package manager automatically. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also write that dbus must be running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with 1b1307a.
Thank you for your feedback! Excepted for a few comments, the most things should be corrected or improved. I will clarify the limitation within this PR. Also thank you for opening #4790 this should be definitely migrated to the new backend which will also allow much more detailed tests. |
So, may I merge this now, @markus2330? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We avoid self-merging except for emergencies (e.g. a PR that fixes CI builds of master). Simply rerequest my review to get the PR eventually merged.
- infos/provides = storage/xfconf | ||
- infos/recommends = | ||
- infos/placements = postgetstorage presetstorage | ||
- infos/status = maintained libc configurable experimental unfinished concept limited memleak |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still open.
As usual, this allows the plugin to read and write to the XFCE configuration. | ||
|
||
You can refer to the [official XFCE documentation of xfconf](https://docs.xfce.org/xfce/xfconf/start) to learn more about it. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template has ## Installation
here, please add this information.
# Backup-and-Restore: user:/tests/xfconf | ||
|
||
# mount the xfwm channel | ||
kdb mount /dev/null /test/xfwm xfconf "channel=xfwm4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must be tests, see doc/TESTING.md line 196:
kdb mount /dev/null /test/xfwm xfconf "channel=xfwm4" | |
kdb mount /dev/null /tests/xfconf/xfwm xfconf "channel=xfwm4" |
Basics
(added as entry in
doc/news/_preparation_next_release.md
whichcontains
_(my name)_
)Please always add something to the release notes.
(first line should have
module: short statement
syntax)close #X
, are in the commit messages.doc/news/_preparation_next_release.md
scripts/dev/reformat-all
Checklist
(not in the PR description)
Review
Labels