Implement print_config_info application command

[NaniNoni]

Describe your changes

I implemented the print_config_info command to allow for easily checking which config files are loaded. This is especially useful when f3d is ran not from the command line.

Issue ticket number and link if any

#2349

Checklist for finalizing the PR

• [ ] I have performed a self-review of my code
• [ ] I have added tests for new features and bugfixes
• [ ] I have added documentation for new features
• [x] If it is a modifying the libf3d API, I have updated bindings
• [x] If it is a modifying the .github/workflows/versions.json, I have updated timestamp

Continuous integration

Please check the checkbox of the CI you want to run, then push again on your branch.

• [x] Style checks
• [x] Fast CI
• [ ] Coverage cached CI
• [ ] Analysis cached CI
• [ ] WASM docker CI
• [ ] Android docker CI
• [ ] macOS Intel cached CI
• [ ] macOS ARM cached CI
• [ ] Windows cached CI
• [ ] Linux cached CI
• [ ] Other cached CI

[snoyer]

Nice! thanks @NaniNoni

As it is, from a user point of view, this could answer "where are the current config files?" but not "where should I create my config file?" as only the existing files are listed?

How about removing the fs::exists check from GetConfigPaths https://github.com/f3d-app/f3d/blob/3bec77e7be4867cd62fe3df7416b8204d38e16ab/application/F3DConfigFileTools.cxx#L66 and handling it in ReadConfigFiles where both existing and non-existing files could be logged accordingly?

[NaniNoni]

looks good!

Please add a test though, you can look at

application/testing/CMakeLists.txt:1101

TestCommandScriptPrintScene and copy the logic.

Thank you, will do

[NaniNoni]

I fixed/implemented the requested changes. However, in the loop over canonical paths, I ended up using debug instead of print(logLevel, ...) due to the large amount of unwanted output when the config files were actually being read when f3d is launched. I think the level of output is appropriate now, but it just makes the logLevel parameter slightly misleading since it no longer applies to all internal log calls.

// Recover an absolute canonical path to config file
try
{
  configPath = fs::canonical(fs::path(configPath)).string();
}
catch (const fs::filesystem_error&)
{
  // This (file not found) error is expected in dry run mode
  if (dryRun)
  {
    f3d::log::print(logLevel, "Found available config location ", configPath.string());
  }
  else
  {
    f3d::log::debug(
      "Configuration file does not exist: ", configPath.string(), " , ignoring it");
  }
  continue;
}

[snoyer]

Would it be better to improve the separation of concerns between GetConfigPaths and ReadConfigFiles instead of trying to include the user-facing logging into ReadConfigFiles with a dry-run param?

The idea would be:

• GetConfigPaths returns both the candidate config files and search directories (whether they exist or not)
• ReadConfigFiles iterates over the result of GetConfigPaths, reading the items that are file files and recursing into the items that are dirs
• the print_config_info command uses only GetConfigPaths to print "found this files, would have used that file, would check that dir, ..." without having to do a full dry run of ReadConfigFiles

[NaniNoni]

Would it be better to improve the separation of concerns between GetConfigPaths and ReadConfigFiles instead of trying to include the user-facing logging into ReadConfigFiles with a dry-run param?

The idea would be:

* `GetConfigPaths` returns both the candidate config files and search directories (whether they exist or not)

* `ReadConfigFiles` iterates over the result of `GetConfigPaths`, reading the items that are file files and recursing into the items that are dirs

* the `print_config_info` command uses only `GetConfigPaths` to print "found this files, would have used that file, would check that dir, ..." without having to do a full dry run of `ReadConfigFiles`

I like this idea a lot

[NaniNoni]

I implemented the requested changes. The only downside I'm aware of is that the print_config_info command considers "config" as the only configSearch query for GetConfigPaths. I think this may cause an issue for different config locations.

std::vector<std::filesystem::path> availableConfigPaths =
        F3DConfigFileTools::GetConfigPaths("config");

[snoyer]

The only downside I'm aware of is that the print_config_info command considers "config" as the only configSearch query for GetConfigPaths

You'd have to track down where the configSearch param comes from for the real GetConfigPaths call and store that somewhere to reuse it in print_config_info. I don't know what the proper way to do it would but I'm sure @mwestphal will

[NaniNoni]

I'm confused by this implementation. It looks to me this will not output anything about config files now ? We want the same or at least a similar output when --verbose is used.

Yes, you're right. That's another problem. Previously to these changes, the GetConfigPaths function checked for file existence and logged in debug if they didn't (because that was expected). Now, the existence check moved to ReadConfigFiles. ReadConfigFiles also had an existence check, but logged the results in error verbosity, since the files checked was supposed to exist at that point.

Now, the existence check in ReadConfigFiles cannot be an error, since it's sort of equivalent to the one we had before in GetConfigPaths. What do you think?

[NaniNoni]

The only downside I'm aware of is that the print_config_info command considers "config" as the only configSearch query for GetConfigPaths

You'd have to track down where the configSearch param comes from for the real GetConfigPaths call and store that somewhere to reuse it in print_config_info. I don't know what the proper way to do it would but I'm sure @mwestphal will

Yes I agree. @mwestphal how would you like this to work?

[mwestphal]

The only downside I'm aware of is that the print_config_info command considers "config" as the only configSearch query for GetConfigPaths

You'd have to track down where the configSearch param comes from for the real GetConfigPaths call and store that somewhere to reuse it in print_config_info. I don't know what the proper way to do it would but I'm sure @mwestphal will

Yes I agree. @mwestphal how would you like this to work?

Alright, actually your implementation is not far off!

How about adding another method:

F3DConfigFileTools::PrintConfigInfo(const std::vector<fs::path>&)

that prints output using log::info like this by checking the file/dir existence:

Candidate config file not found: /etc/f3d/config.json
Candidate config file not found: /etc/f3d/config.d
Candidate config file not found: /usr/share/f3d/configs/config.json
Candidate config file not found: /usr/share/f3d/configs/config.d
Candidate config file not found: /home/glow/dev/f3d/others/F3D-3.2.0-RC2-Linux-x86_64-raytracing/share/f3d/confi
Config file found: /home/glow/dev/f3d/others/F3D-3.2.0-RC2-Linux-x86_64-raytracing/share/f3d/configs/config.d
Candidate config file not found: /home/glow/.config/f3d/config.json
Candidate config file not found: /home/glow/.config/f3d/config.d

And use it in print_config_info always and in F3DConfigFileTools::ReadConfigFiles if verbose == debug.

[mwestphal]

Hi @NaniNoni , how is it going ? Do you need any help ?

[mwestphal]

Hi @NaniNoni , how is it going ? Do you need any help ?

[NaniNoni]

Hi, sorry for the delay. I've been away and then unexpectedly very busy, but I've implemented the changes and it should hopefully be closer to the desired implementation. However, the quoted comment below is still unresolved. How would you like this to work?

The only downside I'm aware of is that the print_config_info command considers "config" as the only configSearch query for GetConfigPaths

You'd have to track down where the configSearch param comes from for the real GetConfigPaths call and store that somewhere to reuse it in print_config_info. I don't know what the proper way to do it would but I'm sure @mwestphal will

[mwestphal]

@NaniNoni you need to rebase on latest master

[mwestphal]

Hi @NaniNoni , need any help moving forward ?

[mwestphal]

Hi @NaniNoni , need any help moving forward ?

[mwestphal]

Hi @NaniNoni , need any help moving forward ?