openbmc / phosphor-led-manager

Apache License 2.0
5 stars 12 forks source link

Passing a custom led yaml path to meson #10

Closed seirespcp closed 3 years ago

seirespcp commented 3 years ago

When phosphor-led-manager was transitioning from autotools to meson, the ability to set your own yaml path through EXTRA_OECONF = "YAML_PATH=${STAGING_DATADIR_NATIVE}/${PN}" was never ported.

Instead, it always uses meson.project_source_root().

There are several machines installing led.yaml to that yaml_path, and they seem to be broken now.

For example. the lastSuccessfulBuild of romulus from jenkins still has the wrong led groups:

root@romulus:~# busctl tree xyz.openbmc_project.LED.GroupManager
-/xyz
  -/xyz/openbmc_project
    -/xyz/openbmc_project/led
      -/xyz/openbmc_project/led/groups
        |-/xyz/openbmc_project/led/groups/bmc_booted
        |-/xyz/openbmc_project/led/groups/enclosure_fault
        |-/xyz/openbmc_project/led/groups/enclosure_identify
        |-/xyz/openbmc_project/led/groups/fan_fault
        |-/xyz/openbmc_project/led/groups/fan_identify
        -/xyz/openbmc_project/led/groups/power_on`

This is the led yaml, and it clearly has more than 6 groups.

Is removing led yaml path option intended and someone's going to update those metas?

williamspatrick commented 3 years ago

In most repositories YAML_PATH is a reference to extra sdbus++-style or phosphor-logging YAML. It seems like this repository is using just a single YAML file? I would suggest using a different variable name for it in the future.

Also, many other repositories handle this not by passing a path out of the "native" tree, but by passing the machine-specific YAML (or JSON) file via SRC_URI. If possible I would recommend going this direction. Using the "native" tree requires special -native recipes which are complex and confusing to most people.

seirespcp commented 3 years ago

There are 14 machine confs that use virtual/phosphor-led-manager-config-native. For compatibility issues, it's better to keep this option. But, yes, it could use a clearer variable name. Getting rid of -native is tempting though.

seirespcp commented 3 years ago

fixed in 220e194628f8594587d11436b6486da8edeef4b6