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

LCDproc ABI changes #2748

Open
markus2330 opened this issue Jun 4, 2019 · 24 comments

Comments

4 participants
@markus2330
Copy link
Contributor

commented Jun 4, 2019

LCDproc upstream wants to make some ABI changes:

lcdproc/lcdproc#74

It makes sense, that we also propose which changes we want to do. When these changes happen together, users have an easier upgrade path (recompile only once).

Do we need more changes than passing the Elektra handle to the plugins?

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Do we need more changes than passing the Elektra handle to the plugins?

I think that is the only change necessary. But the issue you linked concerns the client/server protocol not the driver API, so I'm not sure it is really relevant.

When these changes happen together, users have an easier upgrade path (recompile only once).

That is of course true, but I am not sure the Elektra implementation is ready for a full release.

The biggest disadvantage (which I am not planning to address) is that with the Elektra implementation, all drivers have to be compiled together with the server. The reason for this is that the code generation API provides no mechanism for modular specifications (it requires exactly 1 specification per executable). It would be possible to have modular specifications (I think), but it would require significant extensions of the code generation mechanism.

Of course you could just ignore the Elektra handle and handle the configuration for your driver separately, but that is not a solution.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

drivers have to be compiled together with the server.

How was it before? Does the build system allow to compile a single driver?

That is of course true, but I am not sure the Elektra implementation is ready for a full release.

What is your schedule?

Of course you could just ignore the Elektra handle and handle the configuration for your driver separately, but that is not a solution.

I would also not use it for these drivers, it would be the most modular way, though.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

How was it before?

The drivers (like Elektra's plugins) are loaded via dlopen and the location and filename for an active driver could be configured via the config file. This is still the case (using the KDB instead of the config file). However, the Elektra instance passed to the drivers only 100% of the guarantees for the drivers that are part of the LCDd specification. Other drivers would have to either ignore this Elektra handle and access the KDB or their own configuration system independently, or use the handle but take additional care, that no getter fails (hard to impossible, requires at least mounting an additional spec).

Does the build system allow to compile a single driver?

There are no targets in the root directory for that, but cding into the server/drivers and executing make somedriver.so should produce the desired driver file.

What is your schedule?

Ideally I would like to finish ASAP and only fix problems in the current PR (ElektraInitiative/lcdproc#7), but not add anything. An exception to that is some mechanism to convert old config files. I still need to add that.

I would also not use it for these drivers, it would be the most modular way, though.

I am not sure I understand...

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

There are no targets in the root directory for that, but cding into the server/drivers and executing make somedriver.so should produce the desired driver file.

I think building the drivers separately is not really needed then.

An exception to that is some mechanism to convert old config files. I still need to add that.

Is our parser not able to do parse the old files? Actually it would be nicer if a plugin makes the migration: then people could simply continue to use their old files. Actually, @llukask wants to work exactly on this topic.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

I think building the drivers separately is not really needed then.

It is not really about building the drivers in the official repository separately. The problem is more that there might be users out there that have implemented their own unofficial drivers that might not work anymore. I am not sure whether these third party drivers are an officially supported feature or just a side effect of the current implementation.

Is our parser not able to do parse the old files?

Our INI parser has some problems, but it would probably be able to read the files. However, the format of the configuration was changed to varying degrees to accommodate the code generation.

Actually it would be nicer if a plugin makes the migration: then people could simply continue to use their old files.

Of course that would be nicer, but since their is currently no Elektra feature for that the question is purely theoretical at this point. Also I think we should encourage people to get to know the new format and all of Elektra's nice features, otherwise the update is pointless. Transforming the new format into the old one is also somewhat problematic and would definitely require some hackery.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

It is not really about building the drivers in the official repository separately. The problem is more that there might be users out there that have implemented their own unofficial drivers that might not work anymore. I am not sure whether these third party drivers are an officially supported feature or just a side effect of the current implementation.

Yes, it is a point we should mention in the PR.

Of course that would be nicer, but since their is currently no Elektra feature for that the question is purely theoretical at this point. Also I think we should encourage people to get to know the new format and all of Elektra's nice features, otherwise the update is pointless.

I fully agree. As already said before, ideally we also migrate to YAML. (@sanssecours 😉)

Transforming the new format into the old one is also somewhat problematic and would definitely require some hackery.

The only difference is if the transformation code is not a standalone application but a plugin transforming from the old KeySet to the new one.

The new work will be about a nice language to do such transformations. For LCDproc we could implement it in python or C. As @llukask work will most likely not ready soon but we can discuss it the next meeting. Maybe @llukask can even do a case study, comparing hand-written transformations with the transformation written in his language.

@haraldg

This comment has been minimized.

Copy link

commented Jun 8, 2019

Hi just stumbled across this discussion and thought I should give some input.

I believe you are overthinking things a bit. There is no problem with giving users a bit of a rough migration every decade or so. There is no need to put serious effort into the migration path IMO. OTOH I should remind you, that a requirement for making elektra a dependency of LCDproc is, that elekra is included in major distributions. (Think of debian testing as a good benchmark, though others are important as well.)

Regarding the questions raised above:

There aren't many out of tree drivers because we are fairly liberal with what we merge, but there are some. We try to keep a defined driver API to support this even though there is no lcdproc-dev package providing the header files at the moment. That being said, I don't mind if oot drivers aren't fully integrated via elektra. No need to get distracted for this corner case.

About scheduling: I fully expect to do a release before any elktra specific code is merged, though it would be nice, if we could start merging elektra bits soon after the release. However checking again now, it seems there has been little progress on the distribution support front since we last discussed this, so maybe not realistic.

Also I'd like to see some closer cooperation and coordination between ElektraInitiative and LCDproc. I'd rather review and discuss a PR for some small isolated part of LCDproc like one of the clients before all the work is finalized, than be presented with one monster PR, that will take me two years to review. Also we should get people from the LCDproc community to help with porting some of the drivers - that way more people will get some exposure to elektra too.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

I'd rather review and discuss a PR for some small isolated part of LCDproc like one of the clients before all the work is finalized

You can find the current PR here ElektraInitiative/lcdproc#7. The PR is big in terms of files changed, but the actual changes per file are quite small (with a few exceptions).

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

Hi just stumbled across this discussion and thought I should give some input.

Thank you for jumping in!

I believe you are overthinking things a bit. There is no problem with giving users a bit of a rough migration every decade or so.

Then we can relax more about this.

I was considering here the total amount of work time needed. One person invests X days to write a migration code, and Y people save Z minutes because of this. Of course the question is how big X, Y, and Z is if it makes any sense to do that.

OTOH I should remind you, that a requirement for making elektra a dependency of LCDproc is, that elekra is included in major distributions.

I think we already have very good coverage.

Think of debian testing as a good benchmark, though others are important as well.

The upgrade from 0.8.14 to 0.8.27 (or whatever will be needed for LCDproc) should be fairly easy: especially because we also maintain Debian packages for Elektra's master.

I'd rather review and discuss a PR for some small isolated part of LCDproc

We do not yet have code which successfully starts. The current status is here.

like one of the clients

This is a very good suggestion. @kodebach what do you think?

Also we should get people from the LCDproc community to help with porting some of the drivers - that way more people will get some exposure to elektra too.

Yes, we will definitely need help from the community. There are too many drivers for us to port them all and we do not have the hardware to test if it works.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

We do not yet have code which successfully starts.

Actually it should start and run properly (if your build of Elektra includes #2707). There just isn't a way to migrate old configurations yet, although I am working on this.

This is a very good suggestion. @kodebach what do you think?

I could split the PR up into one for each client and one for the server+drivers (cannot be split because it is just one specification). But for now, I will leave it as one PR, because the development is much easier if I'm just working on a single branch instead of 4.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

Actually it should start and run properly (if your build of Elektra includes #2707). There just isn't a way to migrate old configurations yet, although I am working on this.

Very nice, I merged #2707 right now, so you could make your first PR for LCDproc.

There just isn't a way to migrate old configurations yet, although I am working on this.

This seems to be a low-priority issue. Better to update the tutorial (ElektraInitiative/lcdproc#5) so that people who did not hear yet of Elektra will know how to configure LCDproc.

I could split the PR up into one for each client and one for the server+drivers (cannot be split because it is just one specification).

No, this is not needed. The idea was that you make one PR of one client, obviously the one you tested most. You also do not need to work on this branch, the branch is only for review and for people who want to try it out. The PR should contain the tutorial, though.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

Sadly, I am now fairly certain that this current version is not suitable for a release. It turns out that, because of the massive and non-modular specification for LCDd, every kdb set call to any Key under /sw/lcdproc/lcdd/#0/current takes ~ 4 sec (at least on my machine). I have now moved the parts of the LCDd specification that are for drivers not updated yet into a separate file. This reduces the size of the specification file from nearly 5000 lines to "just" ~ 1800 lines and reduces the time of kdb set calls down to ~ 0.5 sec.

There are also major problems with spec-mount #2760. The closer I get to finishing the code-generation and the LCDproc update, the more I realize how many parts of Elektra don't really deliver on what they promise.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

Sadly, I am now fairly certain that this current version is not suitable for a release. It turns out that, because of the massive and non-modular specification for LCDd [...]

That is why we suggest you to finish one client for now. There you should not have these problems.

There are also major problems with spec-mount #2760.

Validation was also not considered very important by @haraldg. So it should also not hold us back to make the PR for one client now.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

That is why we suggest you to finish one client for now.

On the LCDproc side of things the clients as well as the server core and the drivers I updated are already finished. The remaining problems are on the Elektra side.

Validation was also not considered very important by @haraldg. So it should also not hold us back to make the PR for one client now.

The problem is not the advanced validation. The problem is actually that because of #2760 the type plugin is circumvented in some cases (see below for an example). This means we don't have type-checking during kdb set and also not during elektraOpen. Which in turn means that elektraGet* might fail and crash the program.

If we just want something that works, but we don't care about the guarantees that the highlevel API and code-generation can provide (if properly supported by the rest of Elektra), then the work is basically done. As I stated above, running the clients and the server (with one of the updated drivers) should work fine already. I will provide a tutorial on setting up things tomorrow.


The intended setup of LCDproc (will be automated through the install script) is this:

# assume lcdproc source is in /lcdproc
cd /lcdproc

# build and install LCDproc
make
sudo make install

# mount specifications
sudo kdb mount -R noresolver lcdd_spec.eqd "spec/sw/lcdproc/lcdd/#0/current" specload "app=/usr/local/sbin/LCDd"
sudo kdb mount -R noresolver lcdproc_spec.eqd "spec/sw/lcdproc/lcdproc/#0/current" specload "app=/usr/local/bin/lcdproc"
sudo kdb mount -R noresolver lcdexec_spec.eqd "spec/sw/lcdproc/lcdexec/#0/current" specload "app=/usr/local/bin/lcdexec"
sudo kdb mount -R noresolver lcdvc_spec.eqd "spec/sw/lcdproc/lcdvc/#0/current" specload "app=/usr/local/bin/lcdvc"

# spec-mount specifications
sudo kdb spec-mount "/sw/lcdproc/lcdd/#0/current"
sudo kdb spec-mount "/sw/lcdproc/lcdproc/#0/current"
sudo kdb spec-mount "/sw/lcdproc/lcdexec/#0/current"
sudo kdb spec-mount "/sw/lcdproc/lcdvc/#0/current"

All of this works as expected (apart from the fact that some specifications are limited by the too many plugins error). However after using spec-mount we can only use cascading keys for kdb set using non-cascading keys breaks everything.

The key /sw/lcdproc/lcdproc/#0/current/lcdproc/foreground is boolean. The following fails as we would expect.

kdb set /sw/lcdproc/lcdproc/#0/current/lcdproc/foreground foo
#> Using name user/sw/lcdproc/lcdproc/#0/current/lcdproc/foreground
#> Sorry, module type issued the error 52:
#> error in the type plugin: The value 'foo' of key user/sw/lcdproc/lcdproc/#0/current/lcdproc/foreground could not be normalized (type is 'boolean')

(Note that the user namespace is used)

However, this does not produce an error:

kdb set user/sw/lcdproc/lcdproc/#0/current/lcdproc/foreground foo
#> Create a new key user/sw/lcdproc/lcdproc/#0/current/lcdproc/foreground with string "foo"

Because we successfully set the key to a non-boolean value lcdproc will crash during an elektraGet call.

lcdproc
#> ERROR: The value 'foo' of key 'lcdproc/foreground' could not be converted to type 'boolean'.

The error message is understandable for the user and because lcdproc only accesses the Elektra struct during its startup routine, we still crash immediately. However, the guarantees of the highlevel API and the code-generation (i.e. elektraGet* cannot fail) are still broken.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

On the LCDproc side of things the clients as well as the server core and the drivers I updated are already finished. The remaining problems are on the Elektra side.

Yes, but the performance problem is specific to LCDd. We could and should create a PR before we start profiling and optimizing.

The problem is actually that because of #2760 [...] Which in turn means that elektraGet* might fail and crash the program.

#2760 has nothing to do with this. You can always tamper the config file in some ways. During elektraOpen we also need to check if we have correct config. See #2382, last sentence (Expected behavior: elektraOpen should fail, telling that the type of myint does not match with the build-in spec.). Only then we can provide the guarantees.

Of course #2760 is a serious usability problem and of course it is better if the kdb-commands have a unified interface and errors are always caught earlier (except if someone uses -f). But this problem (not the guarantee) can be addressed with a tutorial: We need to teach people not to directly edit config files to use kdb editor system/.. or kdb set /.... Unfortunately editor has the reverse problem then set: it needs the namespace to work, see #1288.

(Note that the user namespace is used)

If you are root (e.g. sudo) the correct "system" namespace is used. This is also a matter of the tutorial to describe this.

@haraldg

This comment has been minimized.

Copy link

commented Jun 9, 2019

Yes, validation etc are not important. As long as there are no (major) regressions I'm happy to switch to elektra. The new features are a bonus, not a requirement.

I was considering here the total amount of work time needed. One person invests X days to write a migration code, and Y people save Z minutes because of this. Of course the question is how big X, Y, and Z is if it makes any sense to do that.

Then let's find out how big Z is by giving people something to play with ...

OTOH I should remind you, that a requirement for making elektra a dependency of LCDproc is, that elekra is included in major distributions.
I think we already have very good coverage.

We also have lots of users on the BSDs ...

Think of debian testing as a good benchmark, though others are important as well.
The upgrade from 0.8.14 to 0.8.27 (or whatever will be needed for LCDproc) should be fairly easy: especially because we also maintain Debian packages for Elektra's master.

Let's hope this is true. It looks like a lot of work is necessary (even if it is easy work) until a version of elektra useful for LCDproc will enter testing.

I just had a first look at the high level API and some samples of your work. As I feared you have done a lot of grunt work without putting up anything for discussion. It is clear that we are month away from being able to merge this. The divergence, that is going to happen in between, will cause lot's of extra work. OTOH it seems it helped you to find some problems on the elektra side, so maybe it was kind of necessary to do it this way.

I will post some comments about the high level API and the samples of LCDproc code into a new issue.

In any case it would be great if you could give people a branch, that is ready to test, and advertise it on the LCDproc mailing list. This should help with getting feedback from the more obscure systems like the BSDs and also help getting a clear picture about the priorities on the remaining issues.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

Yes, but the performance problem is specific to LCDd.

I would say otherwise. The problem is that the specification cannot be modular and therefore has to be quite big. This can only be solved by modifications to Elektra not to LCDd.

#2760 has nothing to do with this.

It has everything to do with this. The type checking during elektraOpen is achieved via the type plugin. If the type plugin is circumvented type checking fails. I don't intend to change this as it should be quite obvious that disabling the type plugin breaks type checking.

During elektraOpen we also need to check if we have correct config. See #2382, last sentence (Expected behavior: elektraOpen should fail, telling that the type of myint does not match with the build-in spec.). Only then we can provide the guarantees.

This is done via kdbGet and the type plugin. There is no other solution since Elektra doesn't have a type system built in.

If you are root (e.g. sudo) the correct "system" namespace is used.

There is no correct namespace here. The same problem occurs with the system namespace.

sudo kdb set /sw/lcdproc/lcdproc/#0/current/lcdproc/foreground foo
#> Using name system/sw/lcdproc/lcdproc/#0/current/lcdproc/foreground
#> Sorry, module type issued the error 52:
#> error in the type plugin: The value 'foo' of key system/sw/lcdproc/lcdproc/#0/current/lcdproc/foreground could not be normalized (type is 'boolean')

sudo kdb set system/sw/lcdproc/lcdproc/#0/current/lcdproc/foreground
#> Create a new key system/sw/lcdproc/lcdproc/#0/current/lcdproc/foreground with string "foo"

I only noted that the user namespace was chosen, so that it would be obvious that the following lines should have the same result.


As I feared you have done a lot of grunt work without putting up anything for discussion.

To be honest my top priority is producing something I can use for my thesis. I fully expected that changes will be required to actually release this version. And just to be clear, I always intended to get my changes merged (possibly after finishing my thesis). If I had put everything up for discussion I would probably have been slowed down by minor details that are important for a release, but not important for my thesis.

Also I tried to keep everything as similar to the old code as possible. The major changes in lcdexec are simply because it makes using the code-generation API much nicer.

It is clear that we are month away from being able to merge this.

I explicitly stated above that I don't think the current version is suitable for a release. I fully expected this.

In any case it would be great if you could give people a branch, that is ready to test, and advertise it on the LCDproc mailing list.

I will create a PR in the LCDproc repo containing only some or all of the clients and a tutorial on how to use them. This won't showcase all of the features, but it will showcase most of them.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

@haraldg wrote:

We also have lots of users on the BSDs ...

We had several problems with BSDs some time ago but as we now have BSD build servers we are quite confident that BSD is quite well supported.

OTOH it seems it helped you to find some problems on the elektra side, so maybe it was kind of necessary to do it this way.

Yes, it was necessary: The high-level API is a completely different view point, with different behavior and most importantly: much more guarantees. @kodebach now thinks in terms of these guarantees and started many discussions how we can also improve the lower-level parts of Elektra (in particular the spec plugin). These discussions were very fruitful, although we unfortunately do not have the man-power to implement everything soon.

I will post some comments about the high level API and the samples of LCDproc code into a new issue.

Thank you!

also help getting a clear picture about the priorities on the remaining issues.

Yes, especially the priorities will be very important to make @kodebach's work more focused.

@kodebach wrote:

The type checking during elektraOpen is achieved via the type plugin. If the type plugin is circumvented type checking fails.

I created #2765 for further discussion about this guarantee.

And just to be clear, I always intended to get my changes merged (possibly after finishing my thesis).

Thank you very much for this!

If I had put everything up for discussion I would probably have been slowed down by minor details that are important for a release, but not important for my thesis.

I know and highly appreciate this. The point of view from @haraldg, however, is that LCDproc people were mostly left out of the discussion.

I will create a PR in the LCDproc repo containing only some or all of the clients and a tutorial on how to use them. This won't showcase all of the features, but it will showcase most of them.

Perfect! 💖

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

I forgot to comment on an important statement:

@kodebach wrote:

@markus2330 wrote:

Yes, but the performance problem is specific to LCDd.
I would say otherwise. [...] This can only be solved by modifications to Elektra not to LCDd.

Yes, this is exactly the point of everything we are doing here: configuration-related problems should be a problem of Elektra, not of individual applications.

But this does not mean that a small PR is easier to review and might expose less problems.

@haraldg

This comment has been minimized.

Copy link

commented Jun 10, 2019

To be honest my top priority is producing something I can use for my thesis.

Fair enough. I guess I'm seeing things too much from the "let's not do duplicate work" POV.

I fully expected that changes will be required to actually release this version.

It's not (only) about the changes required to the PR. It's also about the fact, that it will take quite some time before a suitable version of elektra will be in enough disributions to do the merge, even if your code is ready per se.

Anyway, thanks for your work and looking forward to your announcment on the mailing list.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Unfortunately, I didn't have time to create a PR and write a tutorial yet. However, I managed to find the minimal set of plugins required for LCDproc.

Using this CMake option should work:

-DPLUGINS="c;cache;dump;gopts;hexnumber;ini;list;mmapstorage;network;ni;noresolver;path;quickdump;range;reference;resolver;resolver_fm_hpu_b;spec;specload;type;validation"

cache and mmapstorage aren't technically required, but they might improve performance. If you disable testing with -DBUILD_TESTING=OFF, building should also be much quicker (#2776 is needed to do this).

@haraldg

This comment has been minimized.

Copy link

commented Jun 11, 2019

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

The main cause seems to be the code inside elektragen.c, but most other object files have increased in size too.

Yes, this increase is heavy, did you find out which symbols are so big? elfdiff or comparision of output from readelf.

Ideally, the wrappers in the code generation should mostly disappear, as they are only for type-safety.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Yes, this increase is heavy, did you find out which symbols are so big?

See #2772 (comment). I suspect a lot of the size increase comes from the strings needed to construct the specification KeySet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.