Implement print_config_info application command #2382

NaniNoni · 2025-07-09T10:47:44Z

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
If it is a modifying the libf3d API, I have updated bindings
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.

mwestphal · 2025-07-09T19:56:34Z

application/F3DConfigFileTools.cxx

+  // Clear config file paths to avoid reading them if dryRun is set
+  if (dryRun)
+  {
+    actualConfigFilePaths.clear();


insteaed you may want to just return ?

Yes I will do that

application/F3DConfigFileTools.h

mwestphal

looks good!

Please add a test though, you can look at

application/testing/CMakeLists.txt:1101

TestCommandScriptPrintScene and copy the logic.

snoyer · 2025-07-10T05:44:13Z

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

f3d/application/F3DConfigFileTools.cxx

Line 66 in 3bec77e

if (fs::exists(configPath))

and handling it in ReadConfigFiles where both existing and non-existing files could be logged accordingly?

NaniNoni · 2025-07-11T21:15:27Z

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 · 2025-07-11T22:13:33Z

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;
}

mwestphal · 2025-07-12T05:28:24Z

application/F3DConfigFileTools.cxx

-        }
+        configPath = dir / configName;
+        paths.emplace_back(configPath);
+        f3d::log::debug("Found potential config file: ", configPath.string());


So you changed the logic a bit so that GetConfigPath does not check existence.

TBH I dont think this is needed. You can control the logLevel instead so it doesnt debug only.

mwestphal

not convinced by the new impl :/

snoyer · 2025-07-12T06:01:13Z

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 · 2025-07-12T07:29:01Z

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

This reverts commit e479b50.

This reverts commit 86989f2.

…into F3DConfigFileTools namespace

…ication command

NaniNoni · 2025-07-14T21:28:53Z

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 · 2025-07-15T04:15:24Z

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

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.

application/F3DConfigFileTools.cxx

mwestphal · 2025-07-15T06:33:27Z

application/F3DConfigFileTools.cxx

-      configPath = fs::canonical(fs::path(configPath)).string();
-    }
-    catch (const fs::filesystem_error&)
+    if (!fs::exists(configPath))


you need a try catch around this call

NaniNoni · 2025-07-15T14:46:31Z

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 · 2025-07-15T14:48:32Z

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 · 2025-07-15T17:15:46Z

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 · 2025-07-24T06:38:57Z

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

mwestphal · 2025-08-20T08:35:08Z

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

NaniNoni · 2025-08-20T10:39:06Z

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 · 2025-08-20T11:32:00Z

application/F3DConfigFileTools.h

+/**
+ * Recover a OS-specific vector of potential config file directories
+ */
+std::vector<std::filesystem::path> GetConfigPaths(const std::string& configSearch);


Suggested change

std::vector<std::filesystem::path> GetConfigPaths(const std::string& configSearch);

std::vector<fs::path> GetConfigPaths(const std::string& configSearch);

mwestphal

It looks good. I found a small issue to fix. Next time you push the CI will run.

snoyer · 2025-08-20T11:58:45Z

application/F3DConfigFileTools.cxx

+void F3DConfigFileTools::PrintConfigInfo(const std::vector<fs::path>& configPaths)
+{
+  for (const fs::path& path : F3DConfigFileTools::GetConfigPaths("config"))
+  {
+    if (fs::exists(path))
+    {
+      f3d::log::info("Config file found: ", path);
+    }
+    else
+    {
+      f3d::log::info("Candidate config file not found: ", path);
+    }
+  }


I'd say we can get rid of this method and log specifically in ReadConfigFiles and the print_config_info command

I mean doing something like

for (const fs::path& path : F3DConfigFileTools::GetConfigPaths("config")) { f3d::log::info("- ", path, fs::exists(path) ? "(found)" : "(not found)"); }

in the command and logging in the existing if (!fs::exists(configPath)) branches in ReadConfigFiles() (and it may not need to be info level there?)

I dont think that works ?

ha right, sure, that works too :)

mwestphal · 2025-08-24T14:40:23Z

@NaniNoni you need to rebase on latest master

mwestphal · 2025-09-10T07:24:19Z

Hi @NaniNoni , need any help moving forward ?

mwestphal · 2025-09-21T06:54:22Z

Hi @NaniNoni , need any help moving forward ?

Implement print_config_info application command

86989f2

mwestphal reviewed Jul 9, 2025

View reviewed changes

application/F3DConfigFileTools.h Outdated Show resolved Hide resolved

mwestphal requested changes Jul 9, 2025

View reviewed changes

NaniNoni added 2 commits July 11, 2025 23:53

print_config_info: change file handling in GetConfigPaths

e479b50

print_config_info: add testing

d69e5f4

mwestphal reviewed Jul 12, 2025

View reviewed changes

mwestphal requested changes Jul 12, 2025

View reviewed changes

NaniNoni added 5 commits July 14, 2025 21:03

Revert "print_config_info: change file handling in GetConfigPaths"

3abfa47

This reverts commit e479b50.

Revert "Implement print_config_info application command"

c9cc4ea

This reverts commit 86989f2.

Create print_config_info application command and move GetConfigPaths …

6fae7f4

…into F3DConfigFileTools namespace

Update file handling in F3DConfigFileTools for print_config_info appl…

88f2a8d

…ication command

Update test TestCommandScriptPrintConfig for print_config_info command

51f4c4a

NaniNoni requested a review from mwestphal July 14, 2025 21:29

mwestphal requested changes Jul 15, 2025

View reviewed changes

add PrintConfigInfo function to F3DConfigFileTools

5c0b73e

mwestphal reviewed Aug 20, 2025

View reviewed changes

mwestphal marked this pull request as ready for review August 20, 2025 11:32

mwestphal requested changes Aug 20, 2025

View reviewed changes

mwestphal requested a review from snoyer August 20, 2025 11:33

snoyer reviewed Aug 20, 2025

View reviewed changes

	std::vector<std::filesystem::path> GetConfigPaths(const std::string& configSearch);
	std::vector<fs::path> GetConfigPaths(const std::string& configSearch);

Uh oh!

Implement print_config_info application command #2382

Are you sure you want to change the base?

Implement print_config_info application command #2382

Uh oh!

Conversation

NaniNoni commented Jul 9, 2025 • edited by mwestphal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Issue ticket number and link if any

Checklist for finalizing the PR

Continuous integration

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mwestphal left a comment

Choose a reason for hiding this comment

Uh oh!

snoyer commented Jul 10, 2025

Uh oh!

NaniNoni commented Jul 11, 2025

Uh oh!

NaniNoni commented Jul 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwestphal left a comment

Choose a reason for hiding this comment

Uh oh!

snoyer commented Jul 12, 2025

Uh oh!

NaniNoni commented Jul 12, 2025

Uh oh!

NaniNoni commented Jul 14, 2025

Uh oh!

snoyer commented Jul 15, 2025

Uh oh!

mwestphal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NaniNoni commented Jul 15, 2025

Uh oh!

NaniNoni commented Jul 15, 2025

Uh oh!

mwestphal commented Jul 15, 2025

Uh oh!

mwestphal commented Jul 24, 2025

Uh oh!

mwestphal commented Aug 20, 2025

Uh oh!

NaniNoni commented Aug 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwestphal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snoyer Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwestphal commented Aug 24, 2025

Uh oh!

mwestphal commented Sep 10, 2025

Uh oh!

mwestphal commented Sep 21, 2025

Uh oh!

Uh oh!

NaniNoni commented Jul 9, 2025 •

edited by mwestphal

Loading

snoyer Aug 20, 2025 •

edited

Loading