Skip to content

Conversation

@patrick96
Copy link
Member

@patrick96 patrick96 commented May 15, 2018

I have taken the first step to implement a new config parser. For now it's
only a skeleton. But it should help fix a bunch of issues and make the
parser as well as the dependency checking for variable references as well as
included files much more robust

It will also support giving more helpful error messages for syntax errors, I
think the current implementationn doesn't say anything about syntax errors.

This will also more clearly define the valid syntax for configuration files.

More specific information about the implementation can be viewed in this comment as well as config_parser.hpp

Should
Fix #412
Fix #783
Fix #954
Fix an issue where any key-value pair where the key starts with inherit gets treated as an inherit directive

And make the following obsolte:
Closes #1032
Closes #784

It should also make it easier to add an inotify watch onto included files,
as described in #675

This will probably also change some of the behavior the current config handler exhibits:

  • key = "" will get treated as an empty string instead of the literal string ""
  • All the following characters will not be allowed inside keys and headers: "'=;#[](){}:.$\% as well as anything for which isspace returns true, this should prevent any quirky behavior when a key or a section name is referenced via ${..}. I have not tested what kind of characters polybar disallows at the moment but I think it doesn't do any checking on the names.
  • All references inside values will be resolved while parsing the config instead of whenever a value is accessed. This shouldn't change that much because right now the references are resolved when the key is first accessed, which is while loading the module, which is also basically at the beginning.
    EDIT: I have decided to only resolve references to variables inside the config while parsing and do the others on the fly as before. This way people can have invalid references in parts of their config that are never used and not always have their bar not start or throw errors.
  • Certain strings will be forbidden as section names: self, root, BAR. Because they have a special meaning inside references and so a section [root] can never be referenced.

I'm not sure how many people are using key names with those special characters or spaces, but this would break all those configs, don't know if this justifies a major release.

Progress of splitting up this PR:

patrick96 added 3 commits May 15, 2018 14:54
This way warnings are colored differently from actual errors in vim
This way we can still run tests with some compiler warnings
First step in the config_parser develoment. Only tests functions that
are easily testable without many outside dependencies. Integration tests
will follow.
@patrick96 patrick96 changed the title New config parser [WIP] New config parser May 18, 2018
patrick96 added 5 commits May 21, 2018 15:10
The sstream test was removed because it only tested standard library
behvaior
Not only trimming based on single character matching but based on a
freely specifiable predicate. Will be used to trim all spaces (based on
isspace)
patrick96 added 10 commits June 1, 2018 18:13
Cleaner to let the caller catch and fill in the line number and file
path
Before, trim would remove all characters that *didn't* match the
predicate and thus the predicate isspace wouldn't work correctly. But
because we used the inverse (isnospace_pred) it all worked out, but if
the function was used with any other function, it wouldn't have given
the desired output
This changes the way the config is invoked. Now main.cpp creates a
config_parser object which then returns the singleton config object from
the parse method. Subsequent calls to config::make will return the
already created config object as before

The config_parser does not yet have all the functionality of the old
parser: `inherit` directives are not yet resolved. Other than that all
the old functionality is implemented (creating sectionmap and applying
include-file)

Any sort of dependency detection (except for include-file) are still
missing
config_parser handles the detection of xrdb references and passes that
info to the config object.

This finally allows us to delete the config::parse_file function because
everything in it has been implemented (except for xrdb detection and
file error handling)
Here, because config depends on x11/resources, which somewhere down the
path includes x11/extensions/all.hpp, we need to also compile the source
files for all the x11 extensions (but only the ones enabled)
This can and will be used for integration tests to test if the new
config parser handles inheritance and includes properly.
@codecov-io
Copy link

codecov-io commented Jun 14, 2018

Codecov Report

Merging #1237 into master will increase coverage by 27.91%.
The diff coverage is 50.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1237       +/-   ##
===========================================
+ Coverage    5.84%   33.75%   +27.91%     
===========================================
  Files         161       39      -122     
  Lines        8918     1487     -7431     
===========================================
- Hits          521      502       -19     
+ Misses       8397      985     -7412
Flag Coverage Δ
#unittests 33.75% <50.33%> (+27.91%) ⬆️
Impacted Files Coverage Δ
include/components/config.hpp 0% <0%> (ø) ⬆️
src/components/config.cpp 1.13% <0%> (+0.4%) ⬆️
src/utils/string.cpp 85.71% <100%> (-7.98%) ⬇️
include/utils/string.hpp 33.33% <100%> (+4.76%) ⬆️
src/components/config_parser.cpp 50% <50%> (ø)
include/components/config_parser.hpp 75% <75%> (ø)
include/common.hpp 0% <0%> (-100%) ⬇️
... and 134 more

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 2268136...f05748e. Read the comment docs.

@patrick96
Copy link
Member Author

I have decided to introduce the new config parser step-by-step over multiple PRs so that we don't have an overwhelmingly large PR with all the changes to review. I will leave this PR open for now for reference.

In a first step, I will open a PR that replaces the config parser but doesn't add any new functionality or fix any bugs. The only thing that will change is the more strict naming and more robust parsing. After that I will bit by bit add new features until the parser is fully implemented.

@swillner
Copy link

swillner commented Sep 19, 2018

I don't want to intrude on your work, but are there plans to change the configuration format to JSON or YAML (I have made very good experiences with jbeder/yaml-cpp)? This would much simplify the specification of lists and eliminate the need to implement a parser just for this project... I would be willing to help there.

@patrick96
Copy link
Member Author

@swillner I have not considered this before. I think the big advantage of the ini format is that it is very easy to use for the users since we actually require them to write the config themselves.
I think we'll stick to the ini format for the time being since it would also break all existing configs to switch formats.

@patrick96 patrick96 modified the milestones: 3.3.0, 3.4.0 Oct 8, 2018
@patrick96 patrick96 removed this from the 3.4.0 milestone Nov 7, 2018
@FichteFoll
Copy link

I believe the toml format is more appropriate for a tool like polybar. It's not as complex as yaml, is similar to ini but properly specified and has proper lists.

@patrick96
Copy link
Member Author

We're not going to switch config formats anytime soon.

patrick96 added a commit that referenced this pull request Aug 6, 2019
This is the next step to merge #1237 in stages.

Currently there are barely any restrictions on how the config can be
written. This causes things like config files with DOS line endings to
not be parsed properly (#1366) because polybar splits by `\n` and when
parsing section headers, it can't deal with the `\r` at the end of the
line and thus doesn't recognize any section headers.

With this PR we introduce some rules as to what characters are allowed
in section names and keys.
Note: When talking about spaces I refer to any character for which
`isspace()` returns `true`.

The rules are as follows:
* A section name or a key name cannot contain any spaces as well as any
of there characters:`"'=;#[](){}:.$\%`
* Spaces at the beginning and end of lines are always ignored when
parsing
* Comment lines start with `;` or `#` and last for the whole line. The
whole line will be ignored by the parser. You cannot start a comment at
the end of a line.
* Section headers have the following form `[HEADER_NAME]`
* Key-value lines look like this:
`KEY_NAME{SPACES}={SPACES}VALUE_STRING` where `{SPACES}` represents any
number of spaces. `VALUE_STRING` can contain any characters. If it is
*surrounded* with double quotes (`"`), those quotes will be removed,
this can be used to add spaces to the beginning or end of the value
* Empty lines are lines with only spaces in them
* If the line has any other form, it is a syntax error

This will introduce the following breaking changes because of how
underdefined the config syntax was before:
* `key = ""` will get treated as an empty string instead of the literal
* string `""`
* Any section or key name with forbidden characters will now be syntax
errors.
* Certain strings will be forbidden as section names: `self`, `root`,
* `BAR`. Because they have a special meaning inside references and so a
* section `[root]` can never be referenced.

This replaces the current parser implementation with a new more robust
one that will later be expanded to also check for dependency cycles and
allow for values that contain references mixed with other strings.

This PR also now expands the config paths given over the command line so
that `--config=~/.config/polybar/config` resolves properly.

Closes #1032
Closes #1694

* config_parser: Add skeleton with tests

First step in the config_parser develoment. Only tests functions that
are easily testable without many outside dependencies. Integration tests
will follow.

* config_parser: Implement parse_header

* config_parser: Implement get_line_type

* feat(string): Add trim functions with predicate

Not only trimming based on single character matching but based on a
freely specifiable predicate. Will be used to trim all spaces (based on
isspace)

* config_parser: Implement parse_key

* config_parser: Implement parse_line for valid lines

* config_parser: Throw exception on invalid lines

* config_parser: Remove line_no and file_index from parse_line

Cleaner to let the caller catch and fill in the line number and file
path

* string: Clear up misleading description of trim

Before, trim would remove all characters that *didn't* match the
predicate and thus the predicate isspace wouldn't work correctly. But
because we used the inverse (isnospace_pred) it all worked out, but if
the function was used with any other function, it wouldn't have given
the desired output

* config_parser: Implement parse_file

* config_parser: Switch operation to config_parser

This changes the way the config is invoked. Now main.cpp creates a
config_parser object which then returns the singleton config object from
the parse method. Subsequent calls to config::make will return the
already created config object as before

The config_parser does not yet have all the functionality of the old
parser: `inherit` directives are not yet resolved. Other than that all
the old functionality is implemented (creating sectionmap and applying
include-file)

Any sort of dependency detection (except for include-file) are still
missing

* config: Move xrm initialization to constructor

config_parser handles the detection of xrdb references and passes that
info to the config object.

This finally allows us to delete the config::parse_file function because
everything in it has been implemented (except for xrdb detection and
file error handling)

* refactor(config_parser): Cleanup

* config_parser: Set config data after initialization

Looks much cleaner this way

* config_parser: Expand include-file paths

* config_parser: Init xrm if the config uses %{xrdb references

* config_parser: Use same type of maps as in old impl

Polybar has some weird, not yet fixed, inheriting behaviour and it
changes depending on the order in which the config stores its data.
Using the same type of maps ensures that the behaviour stays the same.

* refactor(config_parser): Clearer invalid name error message

* config_parser: Don't allow reserved section names

Sections with the names 'self', 'BAR', 'root' could never be referenced
because those strings have a special meaning inside references

* config_parser: Handle inherit directives

This uses the old copy_inherited function, so this still suffers from
crashes if there are cyclic dependencies.
This also fixes the behaviour where any key that starts with 'inherit'
would be treated as an inherit directive

* config_parser: Clearer dependency cycle error message

* refactor(config_parser): Handle file errors when parsing

This removes the need to check if the file exists separately

* fix(config): expand config file path

Now paths using ~ and environment variables can be used as the config
path

* fix(config): Properly recognize xrdb references

* config_parser: Make messages more informative

* doc(config): Improve commenting

Comments now describe what the config_parser actually does instead of
what it will do.

We also now follow the rule that single line comments inside functions
should use `//` comments

* refactor: Move else on same line as curly braces

* fix(config_parser): Don't duplicate paths in `files`

* refactor(config_parser): Use else if for clarity

* fix(config): Undefined behavior in syntax_error

Before the custom what() method produced undefined behavior because the
returned string became invalid once the function returned.

* refactor(config): descriptive name for useless lines

is_valid could easily be confused as meaning syntactically invalid
without it being clarified in a comment

* refactor(config): Use separate strings instead of key_value

Takes just as much space and is much better to read

* fix(config_parser): TestCase -> TestSuite and fix macro call

Ref: #1644

* config_parser: use const string& in method args

* config_parser: Improve comments

* config_parser: Incorporate review comments
@sudoforge
Copy link

sudoforge commented May 23, 2020

Hey @patrick96 -- I've recently sat down and "modularized" my polybar configuration, which led to finding out that included files don't propagate a reload of the configuration like the root conffile does (this would be related to #675, #1909).

It looks like that portion of this effort never landed in trunk -- can you describe whether or not you think completing this effort or tackling #675 independently makes more sense to you? I don't have much experience with CPP and haven't contributed to Polybar before but would like to take a stab at it.

@patrick96
Copy link
Member Author

@sudoforge I think tackling this separately makes perfect sense. It should only tangentially touch the config code (to get all config files that need to be watched). I haven't done any work on that issue at all, so feel free to tackle this yourself. Thanks 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mixed References And Text in Config Values Self referenced colours caused a segfault Nested inherit no longer works?

5 participants