add preservation of comment lines - #20
Conversation
|
You'll need to look into the submission guidelines regarding commit formatting, etc. |
c1e751f to
075d2cb
Compare
92f0747 to
bf0a598
Compare
56181fc to
0cb0626
Compare
f1c8b3c to
db733c0
Compare
This pull request implements preservation of comments in configuration
files.
Comment lines before an entry, and the comment at the end of an entry
are considered to be associated with that entry.
Example:
# this comment line is associated with section
# this comment line is associated with section
config type 'section' # this comment is associated with section
# this comment line is associated with opt
# this comment line is associated with opt
option opt 'val' # this comment is associated with opt
# this comment line is not associated with any entry
Wih option -D, uci works as before and strips comments.
The following commands have enhanced behaviour:
* uci import
preserves comments of unchanged configuration entries, and imports
entries with their associated comments
* uci export
exports entries with their associated comments
* uci commit
preserves comments of unchanged configuration entries when
committing changes with uci commands 'add', 'delete', 'set',
'add_list', and/or 'del_list'.
* uci get '<config>.<section>' '#'
shows the section comment
* uci get '<config>.<section>.<option> '#'
shows the option comment
* uci set '<config>.<section>=<value>' '<comment>'
sets the section value and comment
* uci set '<config>.<section>.option=<value>' '<comment>'
sets the option value and comment
* uci add_list '<config>.<section>.option=<value> '<comment>'
adds a list entry with value and comment
* uci changes
also shows comments
* uci show
does not show comments
* uci batch
supports the above commands
Features:
* Comments are auto-indented if preserved, set or added.
* The new delta file format is both backward compatible and forward
compatible. Older uci versions interpret the addition in the new
format as comment and ignore it.
* Comments at the end of the file are not preserved because they do
not belong to an entry.
An example of a result: the above example after and option 'opt2' has
been added with a comment:
# this comment line is associated with section2
# this comment line is associated with section2
config type section2 # this comment is associated with section 2
# this comment line is associated with opt
# this comment line is associated with opt
option opt 'val' # this comment is associated with opt
# comment line for new option 'opt2'
option opt2 'val2'
Signed-off-by: Bastiaan Stougie <wififreedom2026@protonmail.com>
db733c0 to
7141f01
Compare
|
Before reviewing any code, I have a few fundamentals to be discussed.
How are such comments linked to an entry? I am not aware of any association mechanism. What happens to comments after an option or list is deleted? - I guess they will become orphaned.
Currently, Does this PR make a fundamental change to how uci works? Many complex packages can have hundreds of potential options and lists. It is very common for a large and mostly commented out config file to be installed, with most and sometimes all options/lists described in some detail as a form of documentation. Without some detailed information, IMHO, the proposed changes to uci in this PR will, for a complex package, result in at best, a messy set of unlinked comments, or at worst, a config file that current packages might consider invalid. |
In the config file, the comments are by convention linked to an entry by their position. In memory, uci stores the comments in the uci_element.comment struct member (in uci.h), unless -D has been specified, in which case the behavior is as original uci and no comments are stored in memory while reading a config file.
They are deleted together with the option or list (with the uci_element or uci_elements).
This does not apply, the comments associated with an entry form a whole and are deleted together.
uci strips comments if you commit one or more changes. I believe sometimes, a commit is not required (e.g. if I remember correctly, merge commands in some situations apply changes directly, someone opened an issue about this a while ago, but that's something not relevant to this pull request).
No fundamental change. In the delta file, original uci does not store a '#', but does ignore a '#' and everything after it when reading a delta line. The new code stores comments in the delta file at the end of the line after a '#', and processes them when reading and committing. E.g. at the end of a 'set' delta, or of an 'add_list' delta. So this is both forward and backward compatible. Basically, the only change is that uci no longer ignores everything at the end of an entry after a '#' character, in config files, delta files, and on the command line.
The comments are stored in the delta file with the deltas for the set and add_list commands, and applied together with the rest of the delta.
No unlinked comments, no mess, no invalid config file. Entries are deleted with their comment. A change to an entry deletes both the old entry and all associated comment. No mess remains. No invalid config file can result. |
|
Example delta file with comments: tests/shunit2/references/changes_keep_comments.result Original uci for each line just ignores the first '#' and everything after it. |
I would have to invest a significant time to try to figure out what it is that link is supposed to be and then interpreting it in real terms! You say you have a working version of the new code, so: Tests:
Depending on these results I could suggest more simple tests. I don't expect you to install the full package, but note for information, when the package () service daemon is running. expected outputs should be similar to: For completeness, currently, after startup, or after reflash with the package built in, the actual config file is as follows: Note also: The package installs a copy of the "super-#commented" original elsewhere for user reference. |
|
I still find the whole idea absolutely preposterous!! but if by some miracle it manages to sneak by, the following is a hard NO!!
Default should be the original default!! no flags, you have got this backwards, -C (may be taken) or similar for comment would be appropriate as it is the new feature. It would take me a over a week to update all my existing uci code and modules to specify an option that previously didn't exist. |
Indeed, I would most definitely agree, but reversing it will mean the precious comments would disappear anyway. It is a firm no from me too unless I can be 100% demonstrated that there is no impact elsewhere. It looks to me that this has been developed from the point of view of "manual edits to add comments keep disappearing", in the bubble of experience where the config files are always edited directly, something unfortunately often seen being propagated on the Forum. |
The export has leading whitespace removed from each line, and blank lines removed, so I used
|
|
We can certainly discuss default options. One of the challenges is to have all applications honor the setting. If only one application that uses libuci to make changes to the config files removes the comments, they are gone. So it has to be the default uci behavior, or a global setting for the entire system that libuci can honor. For now, I have chosen the default is to keep comments. For example, I considered having a hidden file like /etc/config/.ucikeepcomment instead. There are many other possibilities that crossed my mind. Let's discuss the feature itself first. |
|
I don't see why it needs to be a new feature of uci. I have dozens of features like this for ubus. Things I prefer and or find extremely handy. But I couldn't imagine proposing to have any of them actually added to ubus. Instead, I simply create a patch and put it in the patches dir in ubus root. If i want to share, i create a fork so others can pull in the patches and test. Sure, i have to maintain it, but that's just part of SD IMO. To me it seems the approach here is to just get it in there so the maintainer will maintain it, with no thought as to how it would affect the 1000's of users that already have uci deployed in projects. The -D is just proof of that. Sure it just becomes default behavior and luci and the like need no modification to accept it ... but this imposes on everyone. So i either just accept it, or go through 1000's of lines of code to add the -D flag to override it !! in what world does that make any sense... |
|
@TheRootED24 |
|
I have no issue with comments in my configs. The problem is, I have been using uci since its inception. I have it deployed on all sorts on tiny embedded devices, many of which have little to no bytes to spare. This could potentially cause all sorts of headaches for me and many others moving forward. UCI is a very mature library, any changes what so ever will be far reaching ...far beyond just comments in a config file |
|
have you considered an external library ?? I have not looked too closely at all the code but from what i did examine, this could be done as an external lib with uci as a dependency .. I guess this would require far more accommodation so again it come back to the question ... is this something that will benefit the many ...or just a few |
What do you mean exactly, libuci.so already exists?
uci is nice for automation, but not for other use cases such as troubleshooting. For example, everyone at the forum asks to cat the config file, not to do 'uci show'. Also, I don't want to have to remember what each option in a config file does and what the possible values are and why I set it to the current value to solve which problem. So I like very much to add a comment and keep that comment. I'm not alone, I'm sure. If I'm the many or the few I can't claim. I don't know how common devices with only bytes to spare are, although with the current AI situation making memory very expensive they may become more popular again. Packages become larger all the time, including the kernel. At some point you may need to upgrade your device anyway, for reasons having nothing to do with the few bytes extra used by uci keeping comments. I will strive to accommodate this use case anyway. We can of course have something other than option "-D". |
doesn't warrant disruption of current deployment, and most users know that
your proving my point here, name 1 packages that has increase size to retain comments ?? or a kernel feature that hasn't been carefully scrutinized with the WHY question. What is your use case, do you have fleets deployed as many openwrt users/devs do ? or a few devices ? something tells me the later, which is likely why you failed to properly consider cost/benefit of such a novelty feature. You can document configs with a pen and paper when it comes down to it, the same cannot be said for memory. In most embedded devices, memory is always the first consideration. I do not argue that it this is a useful feature, the problem is this feature does nothing to enhance the overall functionality of a mature embedded library. It does not add security, does not fix a bug, and cost precious memory, which leaves you standing on its handy for you. As i have said before, there is many other ways this can be implemented without affecting a single users anywhere. You can also use it as you wish. But until you've proven its usefulness or make your case as to why it must be done by altering uci (potentially affecting many users) i see it as a none starter. Which is likely why no one will look at it. To be clear, I think you've done a wonderful job, I know the uci codebase very well and i see no issues with the actual implementation. Just can't get past the WHY With that said, I have stated my case. I do not wish to impede on the conversation any further. I will live with the maintainers decision. kind regards. |
No As I indicated on the forum, a reliable way to include comments is to embed them as list entries. From the forum:
|
No they have not gone, the
|
i missed this in the forum, Now that is a pretty slick and elegant solution implemented with nothing but good thinking . You can even be option specific .. list <enable_server_info> .. list <enable_server_note_special_case> and this works great with uci-defaults as well. case closed |
|
The |
|
yes but you can query the comment directly which is more typical usage |
|
There is no structure. You can add |
|
i think accurate naming solves this directly ...no ? |
|
Even with generic naming it is often good enough: |
|
It's fine if someone wants to use a I don't want the system to delete the syntactically valid comments I add to the config files. I'm sure I'm not the only one to expect that from the system. By the way, do you also delete all comments from the *.sh source code in /lib to save space? |
Are you under the impression uci must be used within openwrt ? i use it on actual embedded devices for config and hot reloads, in many devices i don't have room for the scripts ... let alone the comments ;) |
|
If size really matters, it would even be possible to have A few Of course that comes with its own advantages and disadvantages. |
|
well shoot!! now thats a mod i could get behind |
|
I'm tempted to implement the uci-full and libuci-full package solution, but at the same time wonder if the maintainers would frown at the idea of having to maintain that and all the #ifdef statements. So, what to do? |
|
@wififreedom
I would say a completely independent new package, that reads in the static /etc/config files and builds a config specific "documentation file" every time it is run, allowing people to add their own comments locked in to each option etc. |
|
The core libraries are designed in principle to be very nimble and contain no frills ...frills are user specific. I am sure this would have been added if it was part of the intention of the library ... the author doesn't miss much ;) My advice would be to host a package or fork and offer it that way, users that find it useful will thank you, and those that don't will not want to burn your ram out of spite :)
I agree with this 100%, openwrt its just a bunch of packages, I have near as many of my own packages as openwrt :) i link to libraries, but build on top, and never within the cores. |
|
@bluewavenet @TheRootED24 I do thank you both for your feedback. It's given me more insight, and I have ideas for an implementation that does not get in anyone's way. |
|
I will retract my pull request. The way to go is not by modifying uci and libuci. One of the technical problems I encounter is that the libuci API is too exposed to make non-breaking changes: all structs can be manipulated at will by clients, and modifying a struct breaks binary compatibility, requiring the client to recompile its code. There appears to be much opposition to expand a tool that strives to be as lean and mean as possible for embedded systems. So it seems that '#'-style comments in config files will not be supported for systems that use uci, until uci and libuci can be replaced by something more flexible. I will work on that instead, no results promised. |
|
Closed for the above reasons. |
4 participants
This pull request implements preservation of comments in configuration files.
Comment lines before an entry, and the comment at the end of an entry are considered to be associated with that entry.
Example:
Wih option
-D, uci works as before and strips comments. The following commands have enhanced behaviour:uci importpreserves comments of unchanged configuration entries, and imports entries with their associated commentsuci exportexports entries with their associated commentsuci 'commitpreserves comments of unchanged configuration entries when committing changes with uci commands 'add', 'delete', 'set', 'add_list', and/or 'del_list'.uci get '<config>.<section>' '#'shows the section commentuci get '<config>.<section>.<option> '#'shows the option commentuci set '<config>.<section>=<value>' '<comment>'sets the section value and commentuci set '<config>.<section>.option=<value>' '<comment>'sets the option value and commentuci add_list '<config>.<section>.option=<value> '<comment>'adds a list entry with value and commentuci changesalso shows commentsuci showdoes not show commentsuci batchsupports the above commandsFeatures:
I've added lots of shunit2 tests to verify the implementation.
An example of a result: the above example after and option 'opt2' has been added: