Incorrect code generation for systemd StartTransientUnit

[ritzow]

The code generated by the code generator seems to add an extra parameter, or maybe I'm just confused about what the parameter should contain?

Code generation code snippet:

//schema represents the file "/usr/share/dbus-1/interfaces/org.freedesktop.systemd1.Manager.xml"
var gen = new InterfaceCodeGenerator(true, Optional.ofNullable(schema.file)
    .map(File::toPath).map(path -> {
        try {
            return Files.readString(path, StandardCharsets.UTF_8);
        } catch (IOException e) {
            throw new RuntimeException(e);
        }
    })
    .orElse(null), schema.objectPath, schema.busName, null, true);
gen.analyze(false)

Generated method in the generated class:

public DBusPath StartTransientUnit(String name, String mode, List<StartTransientUnitStruct> properties, List<StartTransientUnitStruct> aux);

Based on this dbus spec:

  <method name="StartTransientUnit">
   <arg type="s" name="name" direction="in"/>
   <arg type="s" name="mode" direction="in"/>
   <arg type="a(sv)" name="properties" direction="in"/>
   <arg type="a(sa(sv))" name="aux" direction="in"/>
   <arg type="o" name="job" direction="out"/>
  </method>

The "properties" parameter should be a key-value pairs array where the key is a string and the value is a Variant. D-Spy lists it as StartTransientUnit(string name, string mode, a(sv) properties, a(sa(sv)) aux) -> (Object Path job) (also not sure why properties and aux are the same in the generated code when they have different dbus signatures).

Here's what a run of systemd-run --user --pty ps aux looks like in Bustle (dbus traffic capture):

Signature ssa(sv)a(sa(sv))

Body (notice that the angle brackets represent a Variant, and the aux param is the empty array starting with @a):

"run-u228.service", "fail", [("Description", <"/usr/bin/ps aux">), ("AddRef", <true>), ("StandardInput", <"tty">), ("StandardOutput", <"tty">), ("StandardError", <"tty">), ("TTYPath", <"/dev/pts/1">), ("Environment", <["TERM=xterm-256color"]>), ("ExecStart", <[("/usr/bin/ps", ["/usr/bin/ps", "aux"], false)]>)], @a(sa(sv)) [])

Generated StartTransientUnitStruct class (see my inserted java comments):

public class StartTransientUnitStruct extends Struct {
    @Position(0)
    private final String member0; //this is the key
    @Position(1)
    private final List<StartTransientUnitStructStruct> member1; //idk what this is supposed to be, this should be a Variant representing the value in the key-value pair, instead it's another generated class (see below).

    public StartTransientUnitStruct(String member0, List<StartTransientUnitStructStruct> member1) {
        this.member0 = member0;
        this.member1 = member1;
    }

    public String getMember0() {
        return member0;
    }

    public List<StartTransientUnitStructStruct> getMember1() {
        return member1;
    }
}

I think StartTransientUnitStruct may actually be the type for the aux array type, but it's using it for both properties and aux? StartTransientUnitStructStruct seems like it's the actual appropriate type for values in the properties array.

Generated StartTransientUnitStructStruct class:

public class StartTransientUnitStructStruct extends Struct {
    @Position(0)
    private final String member0; //key?
    @Position(1)
    private final Variant<?> member1; //this is what we actually want, the value in the systemd key-value pair.

    public StartTransientUnitStructStruct(String member0, Variant<?> member1) {
        this.member0 = member0;
        this.member1 = member1;
    }

    public String getMember0() {
        return member0;
    }

    public Variant<?> getMember1() {
        return member1;
    }
}

Lastly, here's my data types that I'm using for now since the generated code isn't correct:

    DBusPath StartTransientUnit(String name, String mode, List<UnitProperty> properties, List<AuxProperty> aux);

    class UnitProperty extends Struct {
        @Position(0)
        private final String key;
        @Position(1)
        private final Variant<?> value;

        public UnitProperty(String key, Variant<?> value) {
            this.key = key;
            this.value = value;
        }

        public String key() {
            return key;
        }

        public Variant<?> value() {
            return value;
        }
    }

    class AuxProperty extends Struct {
        @Position(0)
        private final String key;
        @Position(1)
        private final List<UnitProperty> properties;

        public AuxProperty(String key, List<UnitProperty> properties) {
            this.key = key;
            this.properties = properties;
        }

        public String key() {
            return key;
        }

        public List<UnitProperty> properties() {
            return properties;
        }
    }

And here's my code to call StartTransientUnit:

systemd_custom.StartTransientUnit(myUnit, "fail", List.of(
                new SystemdDbus.UnitProperty("ExecStart", new Variant<>(
                    List.of(new SystemdDbus.ExecStartParams("/usr/bin/ps", List.of("/usr/bin/ps", "aux"), true)),
                    new DBusListType(SystemdDbus.ExecStartParams.class)))
            ), List.of());

Thank you for your time. Please let me know if I can provide any more information.

dbus-java version info: Commit 0e8a16db97dce8d0c878852f3f26b7e8f01060c2 (built from master 2024-3-24) (5.0.1-SNAPSHOT)

[hypfvieh]

Thanks for the report. The generated classes are all fine. The problem is that the generator did not remember the struct class names it already created. This will then create the same struct class name multiple times when determining the class to use for method signature.

I fixed this in my latest commit.

I also improved the naming for the created struct classes to include the name of the argument/parameter it belongs to. This means you will get a StartTransientUnitAuxStruct and a StartTransientUnitPropertiesStruct class. I think this would improve the understanding on where the struct classes are used instead of naming them StartTransientUnitStruct and StartTransientUnitStructStruct etc.

[ritzow]

Fantastic. I've updated my code to use your latest code and it works perfectly. Thank you!

It isn't a concern for me, but wouldn't the new naming scheme for structs be a backwards-incompatible change? It seems like that could be a suprising change in a minor release. I don't know what your compatibility model for the library is, or if this change only affects the broken codegen anyways.

[hypfvieh]

Yes this is indeed a breaking change, but it only affects the code generator - therefore I'm fine that. Also the breaking change will result in compile errors which are easily to spot and fix (other than various runtime issues you never see in advance).

The InterfaceCodeGenerator is usually used once by a developer to create the required classes/interfaces. After that there is no need to re-run it. The only reason to re-run would be updating the interfaces due to changes on the program exposing those interfaces on DBus. In this case it would be necessary to review and test your own code anyway.

The compatibility model is usually "no breaking changes in same major". But this is only true for the library (core) and the required transports - not for the utils or examples.