Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit a CE_Warning when CPLSet[Thread]ConfigOption() is called with a unknown config option, in debug mode #11230

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rouault
Copy link
Member

@rouault rouault commented Nov 9, 2024

$ gdalinfo --config BAD_OPTION=TEST --debug on --version
Warning 1: Unknown configuration option 'BAD_OPTION'.

@rouault rouault added enhancement funded through GSP Work funded through the GDAL Sponsorship Program labels Nov 9, 2024
@rouault rouault added this to the 3.11.0 milestone Nov 9, 2024
@rouault rouault force-pushed the known_config_options branch 11 times, most recently from e047a7c to fe75cfc Compare November 10, 2024 02:29
@coveralls
Copy link
Collaborator

coveralls commented Nov 10, 2024

Coverage Status

coverage: 73.7% (-0.002%) from 73.702%
when pulling 9bda155 on rouault:known_config_options
into d9b65d7 on OSGeo:master.

@rouault rouault force-pushed the known_config_options branch 2 times, most recently from 74412ec to 6d38f54 Compare November 10, 2024 23:34
@dbaston
Copy link
Member

dbaston commented Nov 11, 2024

I don't know if this is a good idea, just tossing something out there. What if the CLI populates a global set of config options that have been specified. CPLGetConfigOption removes items from that set. On program exit, we check to see if the set was empty, and issue a warning with the names of config options that were specified but never used. This would avoid the need to maintain a header of known options, and also would produce a warning if a user specified an option that is known to GDAL but not applicable to their particular usage.

@rouault
Copy link
Member Author

rouault commented Nov 11, 2024

On program exit, we check to see if the set was empty, and issue a warning with the names of config options that were specified but never used.

There's actually code that essentially does that, currently protected by if #DEBUG_CONFIG_OPTIONS, but at the time I wrote it, I was nervous about the performance implications of enabling that in production builds at each CPLGetConfigOption() access. Perhaps that's a not founded worry, since calling CPLGetConfigOption() has already performance implications, and shouldn't be done in performance sensitive loops.
But more fundamentally users specifying config options that are not going to be read by the code can be OK in some use cases, for example when defining them in the GDAL configuration file (where they can apply to different type of workflows).

@dbaston
Copy link
Member

dbaston commented Nov 13, 2024

If the behavior is opt-in (CPL_DEBUG=ON) then it would be useful to know that a config option you specified was not valid in the context you tried to use it in. (For example, writing a netCDF with BLOCKXSIZE.) If that spits out warnings for various tokens etc. that are stored in a configuration file, I think that's OK. Probably out-of-scope here, though.

@rouault
Copy link
Member Author

rouault commented Nov 13, 2024

If that spits out warnings for various tokens etc. that are stored in a configuration file, I think that's OK

believe we should only warn about things that are proven issues (specifying a non existing config option), otherwise users will dismiss the avalanche of warnings. Or there should be a GDAL_SHOW_UNUSED_CONFIG_OPTIONS option to enable that behavior.
That said there's a blind spot with this PR regarding config options specified as environment variables. I was thinking that we could warn about ones that would be in the GDAL_ or OGR_ namespaces, but didn't pursue this as part of this PR

@jratike80
Copy link
Collaborator

That said there's a blind spot with this PR regarding config options specified as environment variables.

But the options written into the [configoptions] section of the configuration file are checked, right?

@rouault
Copy link
Member Author

rouault commented Nov 13, 2024

But the options written into the [configoptions] section of the configuration file are checked, right?

with this PR, yes they will be checked, as they are set through CPLSetConfigOption():

CPLSetConfigOption(pszKey, pszValue);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement funded through GSP Work funded through the GDAL Sponsorship Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants