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

elektrify lcdproc #1266

Closed
4 of 5 tasks
markus2330 opened this issue Jan 9, 2017 · 15 comments
Closed
4 of 5 tasks

elektrify lcdproc #1266

markus2330 opened this issue Jan 9, 2017 · 15 comments
Assignees

Comments

@markus2330
Copy link
Contributor

markus2330 commented Jan 9, 2017

http://lcdproc.org/ repo at https://github.com/lcdproc/lcdproc

  • replace config+getopt handling in lcdproc with Elektra (goal: get rid of as much code as possible)
  • replace 451 occurrences of egrep -rH 'config_get_(bool|int|tristate|float|string)'
  • tool to set multiple key/values from a template, add template for lcdproc standard tasks (see related issue kdb fstab command is confusing #1188)

obsolete

  • tool to transform Config::Model to Elektra's validation + enrich it with more validation
  • allow to directly display values from KDB on a LCD
@markus2330
Copy link
Contributor Author

The most important goal is: total number of code in lcdproc should be reduced drastically.

Note: If we generate some docu out of specs, make dist might be a good place.

@haraldg
Copy link

haraldg commented Jan 20, 2017

I'm not sure what "reduced drastically" means. Any patch that removes more code than it adds will make me quite happy. What I tried to say yesterday: lcdproc has a very crude configuration system, yet everybody (except the package maintainers) seems to be happy with that. So adding fancy new features around configuration specification probably won't be much of a selling point. Reducing complexity and lines of code to maintain will be very welcome, so I recommend that this should be a focus.

In the todo list above, I don't understand what "tool to set multiple key/values from a template" means.

@markus2330
Copy link
Contributor Author

markus2330 commented Jan 20, 2017

I'm not sure what "reduced drastically" means.

It refers to the 1883 lines from shared/configfile.c and shared/getopt.c which should be "reduced drastically" .

Any patch that removes more code than it adds will make me quite happy.
Reducing complexity and lines of code to maintain will be very welcome, so I recommend that this should be a focus.

Exactly!

yet everybody seems to be happy with that

There is not much that could be done wrong with a typed config_get API. The skip parameter and the tristate type is rarely used, but it seems to not bother the devs. Do you prefer to keep the same API or do you want to get rid of it (next to its implementation)?

Btw. why is the (uncommented) section for every driver in the config file so important if you have hard-coded default values anyway?

In the todo list above, I don't understand what "tool to set multiple key/values from a template" means.

It would be about setting up a driver with one kdb call. So instead of doing kdb set +lcdproc/CwLnx/Model 12232 && kdb set +lcdproc/CwLnx/Device /dev/ttyUSB0 && .. one would do kdb tset CwLnx 12232 /dev/ttyUSB0 20x4 19200 no yes and the template CwLnx would know which keys to set. It is basically syntactic sugar for the command-line but needed in cases where validation rules enforce multiple parameters to be set consistent (if e.g. a specific model would require a specific baud rate).

@haraldg
Copy link

haraldg commented Jan 20, 2017

Do you prefer to keep the same API or do you want to get rid of it (next to its implementation)?

I'd do as much cleanup as possible. Any API that only exists for historic reasons is a perfect candidate to get rid of. Should we need any internal API to interface to elektra, it would be worth considering if elektra isn't missing some convenience API. - I know, updateing all the many drivers to the new configration API will be a lot of boring work. I guess this is a good excuse to finally drop all the deprecated drivers instead of putting work into them. Also IMO there is nothing wrong with mildly pestering the authors of the recently added drivers, to help with the transition.

Btw. why is the (uncommented) section for every driver in the config file so important if you have hard-coded default values anyway?

I don't know what the initial intention has been. I see it primarily as a convenient form of documentation.

As long as we can still ship configuration with reasonable defaults, I think the template tool is low priority.

@markus2330
Copy link
Contributor Author

it would be worth considering if elektra isn't missing some convenience API

An API, like lcdproc currently has, is definitely missing and it is one of our must-haves for 1.0, see #1270. It just means that lcdproc would depend on Elektra 0.8.20 or later. But obviously it reduces even more code.

The alternative would be code-generation: for generated code it does not matter if it directly uses low-level APIs. Any 0.8 Elektra would be okay then.

I know, updateing all the many drivers to the new configration API will be a lot of boring work.

Maybe we write a tool for automatic migration, shouldn't be too hard and would be useful for most of the other projects, too.

I guess this is a good excuse to finally drop all the deprecated drivers instead of putting work into them.

That would be for the lcdproc's community to decide. But you definitely can use the migration as excuse ;)

I don't know what the initial intention has been. I see it primarily as a convenient form of documentation.

If it is documentation for defaults you would put it as comments in the file, wouldn't you? But it is actually not really important, a 3-way merger should be able to add a new section for a new driver easily -- even if the file is heavily edited.

As long as we can still ship configuration with reasonable defaults, I think the template tool is low priority.

Ok, I moved it down in the list.

@dod38fr
Copy link

dod38fr commented Jan 20, 2017

I don't know what the initial intention has been. I see it primarily as a convenient form of documentation.

LCDd.conf is also used by Config::Model to enable lcdproc with cme. Which also enable automatic configuration upgrade on Debian systems.

@antoneliasson
Copy link

As a casual contributor to lcdproc, I'm a bit sceptical of this project. Its feature list looks nice and all, but it seems a bit to me like xkcd: Standards. Is libelektra actually used by any third-party project? apt-cache rdepends {libelektra4,python3-elektra,lua-elektra} tells me no (Ubuntu 16.04). For Fedora, Arch and OpenSUSE libelektra isn't even in the official package repositories, only in third-party and user-maintained repositories. Should lcdproc really be the first project which is packaged for a major distribution that adopts libelektra?

@markus2330
Copy link
Contributor Author

markus2330 commented Jan 21, 2017

Hi antoneliasson,

standards

We use this xkcd in presentations ourselves. Elektra does not invent a new configuration file format but abstracts over them.

any third-party project

Yes, many projects use it. Unfortunately(*), however, mostly within universities and companies. there are already FLOSS applications using it (oyranos, which is not packaged in Debian/Ubuntu thus you cannot find it with rdepends) and others are in progress (#388).

(*) Unfortunately because our main goal is to improve FLOSS, not proprietary software.

Should lcdproc really be the first project which is packaged for a major distribution that adopts libelektra?

Yes, it is early adoption but I think we can both profit from it. For lcdproc it will be a simplification of code while getting many more tools to work with configuration, for Elektra it will improve its adoption and packaging. For oyranos it works well on both sides, see our discussions here in the issue tracker, e.g. #1134.

@haraldg
Copy link

haraldg commented Jan 21, 2017

@antoneliasson I see your point. However, it is not like we are inventing a new thing here, but rather that we get rid of our own custum (and largely unmaintained) code. If you have any alternative to elektra in mind, that would be interesting to hear.

The pros for elektra from my POV are:

  • It supports ini as format, so I expect we can keep the configuration files almost unchanged.
  • Somebody else seems willing to do the work.
  • It is active, well maintained and carefully designed.
  • I'm the maintainer of the OpenWRT package, so I'm already somewhat familiar with it.

@dod38fr
Copy link

dod38fr commented Jan 21, 2017 via email

@antoneliasson
Copy link

I think what my concerns boil down to is this: If Elektra does not take off and achieve world dominance, will we be worse off than before? Do we retain the old way of configuring things, i.e. manually editing a ini file in /etc and then reloading/restarting the system service? Or do things have to be done the Elektra way from now on, by "mounting" configuration files and editing them with kdb or similar tool?

Although if Elektra does take off, I'll be more open to getting rid of old stuff, like lcdproc's not very well structured ini file, and replacing it with a better format.

@markus2330
Copy link
Contributor Author

markus2330 commented Jan 23, 2017

Thanks for the interesting questions!

If Elektra does not take off and achieve world dominance, will we be worse off than before?

Basically this is what we did the last years: Not only offer an API and wait for world dominance but to offer an implementation that can compete with any configuration library out there. We are not completely there yet (there are some details where specific other libraries are better than Elektra in specific points) but these are not points that the current configuration system of lcdproc supports (not even close). And the libraries that can compete with Elektra have a completely different level on which dependencies they have: Elektra is the only one only requiring libc.

Do we retain the old way of configuring things, i.e. manually editing a ini file in /etc

Absolutely, you can think of libelektra as a small library in C that reads the ini file like it was done before. So you would have the same INI file as before.

and then reloading/restarting the system service?

Elektra does not interfere with restarting. It provides some techniques for reloading but they are optional. So you can tell us your requirements of reloading are, or we also can implement it in the same way as it is done currently. (Without further feedback we most likely would simply replace process_command_line and process_configfile in do_reload).

Or do things have to be done the Elektra way from now on, by "mounting" configuration files

Mounting is a new possibility for you to, e.g., use a different configuration file format, directly write into git instead of files, download files from a server instead using a local ones and much more. But for users that want to keep the default (which seems like it will be the same INI file as it is now?), they do not have to bother about Elektra at all.

and editing them with kdb or similar tool?

The kdb editor spawns your favourite editor and aborts on syntax/validation errors. Think of it like visudo. But you do not have to use it--if you prefer a risky lifestyle ;)

Although if Elektra does take off, I'll be more open to getting rid of old stuff, like lcdproc's not very well structured ini file, and replacing it with a better format.

In Elektra all users can select their own favourite format. We already have, e.g. JSON and INI, and we are currently working on full-blown XML (see #1280) and YAML. The default format obviously is something lcdproc can decide (also (re)decide at any later point of time).

EDIT: sudoedit -> visudo

@antoneliasson
Copy link

Great! No more objections from me then, your honor!

markus2330 pushed a commit that referenced this issue Jan 27, 2017
markus2330 pushed a commit that referenced this issue Jan 28, 2017
@markus2330 markus2330 added this to to discuss in lcdproc Feb 13, 2017
@haraldg
Copy link

haraldg commented Mar 7, 2017

In lcdproc we have a discussion, that among other things touches how we might want to store additional information in the config file in the future. If you think this is relevant for your work, please chime in:
lcdproc/lcdproc#49

@markus2330 markus2330 moved this from to discuss to to implement in lcdproc Mar 18, 2017
@markus2330 markus2330 moved this from to implement to to discuss in lcdproc Mar 18, 2017
@markus2330
Copy link
Contributor Author

I made following two points from above obsolete:

  • tool to transform Config::Model to Elektra's validation + enrich it with more validation
  • allow to directly display values from KDB on a LCD

rationale: 1. @Piankero already wrote the specification, 2. is out of scope

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

No branches or pull requests

6 participants