| Message ID | 20250729073201.5369-13-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thanks for the patch. Quoting Milan Zamazal (2025-07-29 16:32:00) > A newly introduced LIBCAMERA_CONFIG_DIR environment variable specifies a > directory or a list of directories separated by colons where to look for > the libcamera configuration file before trying the standard locations. Oh, this is the path portion from the previous patch... imo it should be squashed. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > Documentation/runtime_configuration.rst | 5 +++++ > src/libcamera/global_configuration.cpp | 28 ++++++++++++++----------- > 2 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst > index 71a1a175d..759f6f476 100644 > --- a/Documentation/runtime_configuration.rst > +++ b/Documentation/runtime_configuration.rst > @@ -19,6 +19,11 @@ order: > - LIBCAMERA_SYSCONF_DIR/configuration.yaml > - LIBCAMERA_DATA_DIR/libcamera/configuration.yaml > > +If LIBCAMERA_CONFIG_DIR environment variable is non-empty then it > +specifies additional directories where to look for the configuration > +file, before looking at the standard locations. It can be a single > +directory or multiple directories separated by colons. Since this specifies the directory, maybe the variable from the previous patch should only support filename directly? Hm... I'm thinking that if the user wants to specify which configuration file they want to use, then they'd specify the exact file (like LIBCAMERA_RPI_CONFIG_FILE), or at most a list of files (delimited by : like $PATH). I personally don't see the need for a convenience environment variable just to set purely the name of the file to search for within standard config directories. What do you (or others) think? Thanks, Paul > + > The default name of the configuration file, configuration.yaml, can be > overridden in LIBCAMERA_CONFIG_NAME environment variable. The variable > can specify just an alternative configuration file name to be looked up > diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp > index 68a4731a1..d80f5a158 100644 > --- a/src/libcamera/global_configuration.cpp > +++ b/src/libcamera/global_configuration.cpp > @@ -68,11 +68,6 @@ bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName) > > void GlobalConfiguration::load() > { > - const std::vector<std::filesystem::path> globalConfigurationDirectories = { > - std::filesystem::path(LIBCAMERA_SYSCONF_DIR), > - std::filesystem::path(LIBCAMERA_DATA_DIR), > - }; > - > const char *libcameraConfigName = > utils::secure_getenv("LIBCAMERA_CONFIG_NAME"); > if (libcameraConfigName && libcameraConfigName[0] == '\0') > @@ -90,6 +85,16 @@ void GlobalConfiguration::load() > if (configName.empty()) > configName = std::filesystem::path("configuration.yaml"); > > + std::vector<std::filesystem::path> configurationDirectories; > + > + const char *configDir = utils::secure_getenv("LIBCAMERA_CONFIG_DIR"); > + if (configDir) { > + for (auto const &path : utils::split(configDir, ":")) { > + if (!path.empty()) > + configurationDirectories.push_back(path); > + } > + } > + > std::filesystem::path userConfigurationDirectory; > const char *xdgConfigHome = utils::secure_getenv("XDG_CONFIG_HOME"); > if (xdgConfigHome) { > @@ -100,15 +105,14 @@ void GlobalConfiguration::load() > userConfigurationDirectory = > std::filesystem::path(home) / ".config"; > } > + if (!userConfigurationDirectory.empty()) > + configurationDirectories.push_back( > + userConfigurationDirectory / "libcamera"); > > - if (!userConfigurationDirectory.empty()) { > - std::filesystem::path user_configuration_file = > - userConfigurationDirectory / "libcamera" / configName; > - if (loadFile(user_configuration_file)) > - return; > - } > + configurationDirectories.push_back(LIBCAMERA_SYSCONF_DIR); > + configurationDirectories.push_back(LIBCAMERA_DATA_DIR); > > - for (const auto &path : globalConfigurationDirectories) > + for (const auto &path : configurationDirectories) > if (loadFile(path / configName)) > return; > } > -- > 2.50.1 >
Hi 2025. 09. 09. 15:55 keltezéssel, Paul Elder írta: > Hi Milan, > > Thanks for the patch. > > Quoting Milan Zamazal (2025-07-29 16:32:00) >> A newly introduced LIBCAMERA_CONFIG_DIR environment variable specifies a >> directory or a list of directories separated by colons where to look for >> the libcamera configuration file before trying the standard locations. > > Oh, this is the path portion from the previous patch... imo it should be > squashed. > >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> Documentation/runtime_configuration.rst | 5 +++++ >> src/libcamera/global_configuration.cpp | 28 ++++++++++++++----------- >> 2 files changed, 21 insertions(+), 12 deletions(-) >> >> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst >> index 71a1a175d..759f6f476 100644 >> --- a/Documentation/runtime_configuration.rst >> +++ b/Documentation/runtime_configuration.rst >> @@ -19,6 +19,11 @@ order: >> - LIBCAMERA_SYSCONF_DIR/configuration.yaml >> - LIBCAMERA_DATA_DIR/libcamera/configuration.yaml >> >> +If LIBCAMERA_CONFIG_DIR environment variable is non-empty then it >> +specifies additional directories where to look for the configuration >> +file, before looking at the standard locations. It can be a single >> +directory or multiple directories separated by colons. > > Since this specifies the directory, maybe the variable from the previous patch > should only support filename directly? > > Hm... I'm thinking that if the user wants to specify which configuration file > they want to use, then they'd specify the exact file (like > LIBCAMERA_RPI_CONFIG_FILE), or at most a list of files (delimited by : like > $PATH). I personally don't see the need for a convenience environment variable > just to set purely the name of the file to search for within standard config > directories. > > What do you (or others) think? Some of it was my suggestion. So in summary, my preference is the following: * have both `LIBCAMERA_CONFIG_NAME` and `LIBCAMERA_CONFIG_DIR`; * if $LIBCAMERA_CONFIG_NAME is empty, load nothing; * otherwise if $LIBCAMERA_CONFIG_NAME is an absolute path, load just that; * otherwise look for $LIBCAMERA_CONFIG_NAME in the following directories: * ':' separated list of directories from $LIBCAMERA_CONFIG_DIR, * pre-defined locations. I think all of them have use cases. For example, if you have a set of configuration in your home directory, it's easiest to use `LIBCAMERA_CONFIG_NAME=myconf1` to select between them. Sometimes you might want to quickly try a temporary configuration, in that case using LIBCAMERA_CONFIG_NAME with an absolute path is easiest. And `LIBCAMERA_CONFIG_DIR` is very useful is a meson devenv. Admittedly that's a more developer centric thing, but it still seems nice to support. Regards, Barnabás Pőcze > > > Thanks, > > Paul > >> + >> The default name of the configuration file, configuration.yaml, can be >> overridden in LIBCAMERA_CONFIG_NAME environment variable. The variable >> can specify just an alternative configuration file name to be looked up >> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp >> index 68a4731a1..d80f5a158 100644 >> --- a/src/libcamera/global_configuration.cpp >> +++ b/src/libcamera/global_configuration.cpp >> @@ -68,11 +68,6 @@ bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName) >> >> void GlobalConfiguration::load() >> { >> - const std::vector<std::filesystem::path> globalConfigurationDirectories = { >> - std::filesystem::path(LIBCAMERA_SYSCONF_DIR), >> - std::filesystem::path(LIBCAMERA_DATA_DIR), >> - }; >> - >> const char *libcameraConfigName = >> utils::secure_getenv("LIBCAMERA_CONFIG_NAME"); >> if (libcameraConfigName && libcameraConfigName[0] == '\0') >> @@ -90,6 +85,16 @@ void GlobalConfiguration::load() >> if (configName.empty()) >> configName = std::filesystem::path("configuration.yaml"); >> >> + std::vector<std::filesystem::path> configurationDirectories; >> + >> + const char *configDir = utils::secure_getenv("LIBCAMERA_CONFIG_DIR"); >> + if (configDir) { >> + for (auto const &path : utils::split(configDir, ":")) { >> + if (!path.empty()) >> + configurationDirectories.push_back(path); >> + } >> + } >> + >> std::filesystem::path userConfigurationDirectory; >> const char *xdgConfigHome = utils::secure_getenv("XDG_CONFIG_HOME"); >> if (xdgConfigHome) { >> @@ -100,15 +105,14 @@ void GlobalConfiguration::load() >> userConfigurationDirectory = >> std::filesystem::path(home) / ".config"; >> } >> + if (!userConfigurationDirectory.empty()) >> + configurationDirectories.push_back( >> + userConfigurationDirectory / "libcamera"); >> >> - if (!userConfigurationDirectory.empty()) { >> - std::filesystem::path user_configuration_file = >> - userConfigurationDirectory / "libcamera" / configName; >> - if (loadFile(user_configuration_file)) >> - return; >> - } >> + configurationDirectories.push_back(LIBCAMERA_SYSCONF_DIR); >> + configurationDirectories.push_back(LIBCAMERA_DATA_DIR); >> >> - for (const auto &path : globalConfigurationDirectories) >> + for (const auto &path : configurationDirectories) >> if (loadFile(path / configName)) >> return; >> } >> -- >> 2.50.1 >>
Quoting Barnabás Pőcze (2025-09-09 23:11:19) > Hi > > 2025. 09. 09. 15:55 keltezéssel, Paul Elder írta: > > Hi Milan, > > > > Thanks for the patch. > > > > Quoting Milan Zamazal (2025-07-29 16:32:00) > >> A newly introduced LIBCAMERA_CONFIG_DIR environment variable specifies a > >> directory or a list of directories separated by colons where to look for > >> the libcamera configuration file before trying the standard locations. > > > > Oh, this is the path portion from the previous patch... imo it should be > > squashed. > > > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> Documentation/runtime_configuration.rst | 5 +++++ > >> src/libcamera/global_configuration.cpp | 28 ++++++++++++++----------- > >> 2 files changed, 21 insertions(+), 12 deletions(-) > >> > >> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst > >> index 71a1a175d..759f6f476 100644 > >> --- a/Documentation/runtime_configuration.rst > >> +++ b/Documentation/runtime_configuration.rst > >> @@ -19,6 +19,11 @@ order: > >> - LIBCAMERA_SYSCONF_DIR/configuration.yaml > >> - LIBCAMERA_DATA_DIR/libcamera/configuration.yaml > >> > >> +If LIBCAMERA_CONFIG_DIR environment variable is non-empty then it > >> +specifies additional directories where to look for the configuration > >> +file, before looking at the standard locations. It can be a single > >> +directory or multiple directories separated by colons. > > > > Since this specifies the directory, maybe the variable from the previous patch > > should only support filename directly? > > > > Hm... I'm thinking that if the user wants to specify which configuration file > > they want to use, then they'd specify the exact file (like > > LIBCAMERA_RPI_CONFIG_FILE), or at most a list of files (delimited by : like > > $PATH). I personally don't see the need for a convenience environment variable > > just to set purely the name of the file to search for within standard config > > directories. > > > > What do you (or others) think? > > Some of it was my suggestion. So in summary, my preference is the following: > > * have both `LIBCAMERA_CONFIG_NAME` and `LIBCAMERA_CONFIG_DIR`; > * if $LIBCAMERA_CONFIG_NAME is empty, load nothing; > * otherwise if $LIBCAMERA_CONFIG_NAME is an absolute path, load just that; > * otherwise look for $LIBCAMERA_CONFIG_NAME in the following directories: > * ':' separated list of directories from $LIBCAMERA_CONFIG_DIR, > * pre-defined locations. > > I think all of them have use cases. For example, if you have a set of configuration > in your home directory, it's easiest to use `LIBCAMERA_CONFIG_NAME=myconf1` to select > between them. > > Sometimes you might want to quickly try a temporary configuration, in that case > using LIBCAMERA_CONFIG_NAME with an absolute path is easiest. > > And `LIBCAMERA_CONFIG_DIR` is very useful is a meson devenv. Admittedly that's a more > developer centric thing, but it still seems nice to support. Hm ok, that makes sense. Then I'd like the above documented more clearly in the documentation (actually the above description is good so even copy&paste (if Barnabás approves)), and squashed. Thanks, Paul > > > Regards, > Barnabás Pőcze > > > > > > > > Thanks, > > > > Paul > > > >> + > >> The default name of the configuration file, configuration.yaml, can be > >> overridden in LIBCAMERA_CONFIG_NAME environment variable. The variable > >> can specify just an alternative configuration file name to be looked up > >> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp > >> index 68a4731a1..d80f5a158 100644 > >> --- a/src/libcamera/global_configuration.cpp > >> +++ b/src/libcamera/global_configuration.cpp > >> @@ -68,11 +68,6 @@ bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName) > >> > >> void GlobalConfiguration::load() > >> { > >> - const std::vector<std::filesystem::path> globalConfigurationDirectories = { > >> - std::filesystem::path(LIBCAMERA_SYSCONF_DIR), > >> - std::filesystem::path(LIBCAMERA_DATA_DIR), > >> - }; > >> - > >> const char *libcameraConfigName = > >> utils::secure_getenv("LIBCAMERA_CONFIG_NAME"); > >> if (libcameraConfigName && libcameraConfigName[0] == '\0') > >> @@ -90,6 +85,16 @@ void GlobalConfiguration::load() > >> if (configName.empty()) > >> configName = std::filesystem::path("configuration.yaml"); > >> > >> + std::vector<std::filesystem::path> configurationDirectories; > >> + > >> + const char *configDir = utils::secure_getenv("LIBCAMERA_CONFIG_DIR"); > >> + if (configDir) { > >> + for (auto const &path : utils::split(configDir, ":")) { > >> + if (!path.empty()) > >> + configurationDirectories.push_back(path); > >> + } > >> + } > >> + > >> std::filesystem::path userConfigurationDirectory; > >> const char *xdgConfigHome = utils::secure_getenv("XDG_CONFIG_HOME"); > >> if (xdgConfigHome) { > >> @@ -100,15 +105,14 @@ void GlobalConfiguration::load() > >> userConfigurationDirectory = > >> std::filesystem::path(home) / ".config"; > >> } > >> + if (!userConfigurationDirectory.empty()) > >> + configurationDirectories.push_back( > >> + userConfigurationDirectory / "libcamera"); > >> > >> - if (!userConfigurationDirectory.empty()) { > >> - std::filesystem::path user_configuration_file = > >> - userConfigurationDirectory / "libcamera" / configName; > >> - if (loadFile(user_configuration_file)) > >> - return; > >> - } > >> + configurationDirectories.push_back(LIBCAMERA_SYSCONF_DIR); > >> + configurationDirectories.push_back(LIBCAMERA_DATA_DIR); > >> > >> - for (const auto &path : globalConfigurationDirectories) > >> + for (const auto &path : configurationDirectories) > >> if (loadFile(path / configName)) > >> return; > >> } > >> -- > >> 2.50.1 > >> >
diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst index 71a1a175d..759f6f476 100644 --- a/Documentation/runtime_configuration.rst +++ b/Documentation/runtime_configuration.rst @@ -19,6 +19,11 @@ order: - LIBCAMERA_SYSCONF_DIR/configuration.yaml - LIBCAMERA_DATA_DIR/libcamera/configuration.yaml +If LIBCAMERA_CONFIG_DIR environment variable is non-empty then it +specifies additional directories where to look for the configuration +file, before looking at the standard locations. It can be a single +directory or multiple directories separated by colons. + The default name of the configuration file, configuration.yaml, can be overridden in LIBCAMERA_CONFIG_NAME environment variable. The variable can specify just an alternative configuration file name to be looked up diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp index 68a4731a1..d80f5a158 100644 --- a/src/libcamera/global_configuration.cpp +++ b/src/libcamera/global_configuration.cpp @@ -68,11 +68,6 @@ bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName) void GlobalConfiguration::load() { - const std::vector<std::filesystem::path> globalConfigurationDirectories = { - std::filesystem::path(LIBCAMERA_SYSCONF_DIR), - std::filesystem::path(LIBCAMERA_DATA_DIR), - }; - const char *libcameraConfigName = utils::secure_getenv("LIBCAMERA_CONFIG_NAME"); if (libcameraConfigName && libcameraConfigName[0] == '\0') @@ -90,6 +85,16 @@ void GlobalConfiguration::load() if (configName.empty()) configName = std::filesystem::path("configuration.yaml"); + std::vector<std::filesystem::path> configurationDirectories; + + const char *configDir = utils::secure_getenv("LIBCAMERA_CONFIG_DIR"); + if (configDir) { + for (auto const &path : utils::split(configDir, ":")) { + if (!path.empty()) + configurationDirectories.push_back(path); + } + } + std::filesystem::path userConfigurationDirectory; const char *xdgConfigHome = utils::secure_getenv("XDG_CONFIG_HOME"); if (xdgConfigHome) { @@ -100,15 +105,14 @@ void GlobalConfiguration::load() userConfigurationDirectory = std::filesystem::path(home) / ".config"; } + if (!userConfigurationDirectory.empty()) + configurationDirectories.push_back( + userConfigurationDirectory / "libcamera"); - if (!userConfigurationDirectory.empty()) { - std::filesystem::path user_configuration_file = - userConfigurationDirectory / "libcamera" / configName; - if (loadFile(user_configuration_file)) - return; - } + configurationDirectories.push_back(LIBCAMERA_SYSCONF_DIR); + configurationDirectories.push_back(LIBCAMERA_DATA_DIR); - for (const auto &path : globalConfigurationDirectories) + for (const auto &path : configurationDirectories) if (loadFile(path / configName)) return; }
A newly introduced LIBCAMERA_CONFIG_DIR environment variable specifies a directory or a list of directories separated by colons where to look for the libcamera configuration file before trying the standard locations. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- Documentation/runtime_configuration.rst | 5 +++++ src/libcamera/global_configuration.cpp | 28 ++++++++++++++----------- 2 files changed, 21 insertions(+), 12 deletions(-)