SUSE / klp-build

The kernel livepatching creation tool
GNU General Public License v2.0
8 stars 2 forks source link

Per user configuration file #10

Closed fgyanz closed 3 weeks ago

fgyanz commented 1 month ago

Hello team,

I would like to discuss here about the possibility of having a conf file as a more advanced and user-friendly configuration mechanism than just using environment variables.

As already described, we currently support a fair number of environment variables to specify the paths of needed dependencies, but other than that we don't have a way to change klp-build's default behavior. We have many hard-coded values based on our own setups and needs, which may differ from the ones from other possible users.

As for the implementation details, here are a few ideas:

Example:

work_dir = ~/myworkdir
data_dir = ~/mydatadir
test_dir = ~/mytestdir
kernel_sources = ~/mykernelsrc
ccp_dirs = ~/myccpdir
kgr_patchs_dir = ~/mykgrdir 
...
# all | x86_64 | ppc etc...
supported_arch = x86_64
# default_ce = y
default_ccp = y
# Enable/disable SLE specific features
suse_feature=true
...

What do you think? It would be great to add more ideas and agree on a final design :)

Thanks, Fernando

marcosps commented 1 month ago

I agree with you, we should have a file like this to avoid having such decisions base code alone.

I would suggest using a json file, since we are already dealing with json for the other configuration files per livepatch project.

Now, related to the values of the file, I would suggest to first add support to the paths of some directories required for our SLE klp-build usage, like test_dir, ccp_dir (which at this point we should just install it..., and the same applies to clang-extract...). The architectures supported shouldn't be stated here, since it changes from patch to patch (there are features that are one enabled on a specific arch, so we should inform it when calling klp-build setup).

About ccp vs ce, we don't create production LPs using ce at this point, so we default to ccp (this is currently true for klp-build), so ATM I don't see much value adding a flag for it.

What do you think about introducing the config file for the paths alone at this point? We can discuss more about the other suggested options after the first change is merged, so we can simplify config.py to use the file and only have the klp-build arguments to overwrite these defaults.

fgyanz commented 1 month ago

What do you think about introducing the config file for the paths alone at this point? We can discuss more about the other suggested options after the first change is merged, so we can simplify config.py to use the file and only have the klp-build arguments to overwrite these defaults.

Absolutely, I totally agree with you!

I would suggest using a json file, since we are already dealing with json for the other configuration files per livepatch project.

You are right, a json file would indeed be easier to implement, integrate and test, while also keeping consistency with the rest of our files. However, due to the nature of the file (a user config, that is), I'm not entirely sure that json is the most user-friendly option. I mean, json is great for rather complex structures that we want to store/share, but in this case we will most likely only need a <variable>=<value> format, right? For me, the most important thing is that the config file comes in the most approachable format as possible: easy to read, modify and use.

For instance, my osc conf file: ~/.config/osc/oscrc

# see oscrc(5) man page for the full list of available options

[general]
# Default URL to the API server.
# Credentials and other `apiurl` specific settings must be configured in a `[$apiurl]` config section.
apiurl=api.suse.de

[api.suse.de]
# aliases=
# user=
# pass=
# credentials_mgr_class=osc.credentials...

[https://api.suse.de]
user=myuser
sshkey=mysshkey
credentials_mgr_class=osc.credentials.TransientCredentialsManager

Both options have their own advantages/disadvantages, and in the end it's a matter of priorities. What do you think?

marcosps commented 1 month ago

Well, having a simple json would not be difficult either:

{
   "workdir" : "~/myworkdir"
}

And we can also install a dummy file when installing klp-build, is one isn't already installed on the user's machine. IHMO this would be easier, and it's pretty easy to just load the json file on python and access it like a Dict, which isn't different from what we do currently for other files created by klp-build itself.

We can sanitize the json file contents, instructing the user to change it to point to where they need. Well, I have a biased opinion about it, since I believe that python + json for storing data is so damn easy and a no brainer :)

fgyanz commented 1 month ago

Well, having a simple json would not be difficult either:

Right, but my concern is more with having brackets and quotes, which is not as simple as just key=value. Even if we provide a dummy file, the user might mess with a quote, making klp-build fail on the next run. Also json doesn't allow having comments AFAIK, so there's that.

IHMO this would be easier, and it's pretty easy to just load the json file on python and access it like a Dict, which isn't different from what we do currently for other files created by klp-build itself.

That's true, from a developer perspective it's somewhat easier to work with :) But as said, IMHO, as a user interface, json is not my cup of tea. However, it might be a "me" problem, and I'm not against using it.

Alright, as soon as I have some spare time I'll start working on the json implementation. I'll try to also have some tests ready too. Thanks a lot for your feedback! I really appreciate it :)

marcosps commented 1 month ago

Well, having a simple json would not be difficult either:

Right, but my concern is more with having brackets and quotes, which is not as simple as just key=value. Even if we provide a dummy file, the user might mess with a quote, making klp-build fail on the next run. Also json doesn't allow having comments AFAIK, so there's that.

Well, I could argue that a config file on the users' .config dir would need some documentation either way, so in the end, for a "tradicional" config file with key and value would be exactly as difficult as a json, if we don't guide the user :)

IHMO this would be easier, and it's pretty easy to just load the json file on python and access it like a Dict, which isn't different from what we do currently for other files created by klp-build itself.

That's true, from a developer perspective it's somewhat easier to work with :) But as said, IMHO, as a user interface, json is not my cup of tea. However, it might be a "me" problem, and I'm not against using it.

Alright, as soon as I have some spare time I'll start working on the json implementation. I'll try to also have some tests ready too. Thanks a lot for your feedback! I really appreciate it :)

My biggest point against having a config file using key=val is that we should then implement something on klp-build to parse it, while using json it's just json.loads as we have a dict :)

marcosps commented 3 weeks ago

Merged! Thanks!