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

Config files #281

Merged
merged 3 commits into from Oct 11, 2018

Conversation

@latk
Copy link
Member

latk commented Sep 29, 2018

This PR implements config files, based upon issue #229 (also addresses #167). The syntax uses an INI-like format.

Thanks

Thank you @thejk for raising the issue of config files and help discover the complexity of this task by writing a few prototypes.
Thank you @Spacetown for writing a prototype that explored a different direction.
Thank you @stadelmanma for doing the hard work of building an abstraction layer above argparse and for advocating for simplicity.

There are still a lot ideas that have not been addressed, but I hope this PR implements most use cases.

Syntax and design decisions.

  • Entries look like key = value. This is handled very similar to a CLI argument --key=value. It is also possible to provide a config option multiple times, or to provide the same argument through a config entry and the CLI arguments. In either case this behaves as if all entries had been on the command line.

  • # comments are supported, empty lines are ignored.

  • Boolean options use the special value “yes” and “no”. For example, the CLI argument --verbose would be configured as verbose = yes.

    Minor restriction: there is currently no way to disable boolean config entries from the command line. E.g. to turn off verbose = yes a --no-verbose option would be needed. (TODO: these can be autogenerated.)

  • In most cases, the config key will be the same as the long option name, without leading dashes. So --html-medium-threshold=50 can be configured via html-medium-threshold = 50. Where the config key differs, this is shown in the autogenerated --help message.

  • The syntax is intentionally restricted in order to ensure forward compatibility with possible future extensions.

    • Common INI file features like sections, semicolon comments, multiline values, quoted values and so on are not supported.
    • Some syntax like quoted values key = "value" and variable substitutions key = $variable will generate an error message.
  • The config file name can be given via --config or is tried as gcovr.cfg in the --root directory.

Review goals

Please review this PR, especially the proposed config system

  • Are there huge problems with the config file syntax that could lead to problems in the future?
  • Are there problems with the config semantics? In particular, that config entries are treated like command line arguments that are inserted before other arguments? (E.g. compare the --no-verbose problem above.)
  • Is the gcovr.cfg name sensible or should it be a hidden file like .gcovrrc?
  • The config keys are mostly autogenerated from long option names, but some were adapted. Are there any problems with these names? The changes: search_paths option uses search-path, --config has no key, -b/--show-branch is called txt-branch, --keep/--delete are renamed to keep-gcov-files and delete-gcov-files, -j is called gcov-parallel in the config file.

Why not an existing parser?

  • Many config parsers are extremely feature rich. Once a feature is supported it cannot reasonably be deprecated in case some features turn out to be a mistake. E.g. YAML has some corner cases that more or less lock you in to a specific YAML version and parser.

  • Other parsers lack some desirable features. The ConfigParser INI format does not support multiple values per key. TOML does not have convenient syntax for a top-level list of strings/regexes.

  • Most formats come with their own data model which complicates parsing. Here, everything is a literal string, though sometimes the values “yes” and “no” have special treatment.

  • Custom parsers allow way better error reports. For example, gcovr will report the line number when a conversion fails:

    ValueError: gcovr.cfg: 2: verbose: boolean option must be "yes" or "no"
    

    Syntax errors also show the offending line:

    SyntaxError: gcovr.cfg: 3: variable substitution syntax (${var}, $(var), or $var) is reserved
    on this line: output = $BUILD/coverage.html
    
  • The code and tests for the syntax-level parser are only a small part of this PR, most of the effort is combining the data from the config file with an argparse-compatible model.

Security considerations

Gcovr might be used with untrusted projects, and will now implicitly load configuration from such projects. Is this safe? The impact of loading a config file is comparable to a Makefile. E.g. the gcov-executable option can lead to arbitrary code execution. But that project already contains arbitrary code, so I don't think this matters.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 29, 2018

Codecov Report

Merging #281 into master will increase coverage by 0.57%.
The diff coverage is 98.4%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #281      +/-   ##
=========================================
+ Coverage   94.22%   94.8%   +0.57%     
=========================================
  Files          14      15       +1     
  Lines        1507    1751     +244     
  Branches      260     309      +49     
=========================================
+ Hits         1420    1660     +240     
- Misses         43      44       +1     
- Partials       44      47       +3
Impacted Files Coverage Δ
gcovr/tests/test_config.py 100% <100%> (ø)
gcovr/__main__.py 93.79% <92%> (-0.66%) ⬇️
gcovr/configuration.py 98.09% <98.57%> (+0.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c0d81f...096a0e4. Read the comment docs.

doc/source/guide.rst Outdated Show resolved Hide resolved
@latk latk force-pushed the latk:config-file branch from 28de2c9 to a551804 Oct 11, 2018
@latk latk force-pushed the latk:config-file branch from a551804 to 096a0e4 Oct 11, 2018
@latk latk merged commit 096a0e4 into gcovr:master Oct 11, 2018
4 checks passed
4 checks passed
codecov/patch 98.4% of diff hit (target 94.22%)
Details
codecov/project 94.8% (+0.57%) compared to 6c0d81f
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@latk latk deleted the latk:config-file branch Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.