-
Notifications
You must be signed in to change notification settings - Fork 229
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
add support for struct entities in MakeVariant
#329
base: master
Are you sure you want to change the base?
Conversation
@guelfey PTAL |
Generally makes sense, however I'd like to stick to the GVariant format from GLib since this seems to be the convention for other tools, and DBus structs are called "tuples" there: https://www.freedesktop.org/software/gstreamer-sdk/data/docs/2012.5/glib/gvariant-text.html#gvariant-text-tuples ar the best docs I can find. So normal parantheses and separating the field with a comma. |
@guelfey ok thanks for the feedback. I wasn't sure what the format should be so I'll repush with those changes tonight. |
@giuseppe, using this PR in c/common the unit file generated looks like
which is incorrectly adding an extra blank line, nothing is being added to io.max. The final property being provided is |
how does the generated value you pass to systemd look like? Not sure if the empty |
@giuseppe running something like:
prints:
as a matter of fact, the extra lines are added to the unit file even without these modifications... |
@guelfey this should be good to go! works with my PR in containers/common |
currently, structs are not parsed properly since `format()` has no handling for the reflect type add a switch case to handle structs and encode some sane defaults if given an empty one resolves godbus#328 Signed-off-by: Charlie Doern <cdoern@redhat.com>
@guelfey sorry I took so long here. To answer your question about why this is needed, mainly for testing and verification that systemd is getting the right values before we merge something into containers/common. Otherwise, debug statements just print |
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!
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.
Test is not passing due to an extra space though 😅 https://github.com/godbus/dbus/runs/7855299917?check_suite_focus=true#step:6:252
currently, structs are not parsed properly since
format()
has no handling for the reflect type.add a switch case to handle structs and encode some sane defaults if given an empty one
resolves #328
Signed-off-by: Charlie Doern cdoern@redhat.com