config: do not stop if the .config file is present

[matttbe]

virtme-configkernel didn't do anything if the .config file was present, even when launched with --update.

Now, it always updates it with the given config, and it overrides it if asked (e.g. --defconfig).

[arighi]

I like the idea to overwrite the config with --defconfig (or --allnoconfig), but appending the virtme configs with --update is going to break some workflows. People may want to generate a config, then apply some changes (even change some virtme default configs) and keep the config.

To avoid breaking this case the default behavior should be to not touch the .config if it exists. How about introducing a separate option for virtme-configkernel? Something like --force-update for example?

[matttbe]

I like the idea to overwrite the config with --defconfig (or --allnoconfig), but appending the virtme configs with --update is going to break some workflows. People may want to generate a config, then apply some changes (even change some virtme default configs) and keep the config.

To avoid breaking this case the default behavior should be to not touch the .config if it exists. How about introducing a separate option for virtme-configkernel? Something like --force-update for example?

OK, I think I understand your concern. The behaviour of virtme-configkernel --update is different than in the old virtme, but I'm fine to have it changed. Still, it feels strange that the --update option is not updating the .config file if it exists, no?

I guess my modification might be a problem only for people using virtme-configkernel via vng, no? In this case, should we not do a special case for vng? Like calling virtme-configkernel with a new option (--create?) which will not do anything if the .config already exists?

One last thing, if I understand the code correctly, and linked to what I wrote in my first commit, if vng is called a second time with --build or --kconfig, but this time with --config option, the new expected options will not be added, right? Or if vng is used for the first time but an old .config file is present, it will potentially not add the required options, and the kernel will not boot, right?

It feels like something is missing here. Should vng not call virtme-configkernel if the config file exists, and warn the user? Or block and ask what to do in vng if the .config already exists?

[arighi]

I like the idea to warn the user if the config is not touched when --kconfig is passed. I don't like to block and ask for action, because vng is used a lot in automated scripts, so we definitely don't want to ask for interactive actions there.

I'm also not opposed to introduce a new --create command, specifically to the usage required by vng.

In general I think the behavior should be the following:

• do not overwrite .config if it already exists (maybe warn the user in this case)
• allow to overwrite .config if --force (or an equivalent "force" option is passed)
• no interactive questions"

What do you think?

[matttbe]

Thank you for your reply.

In general I think the behavior should be the following:

* do not overwrite `.config` if it already exists (maybe warn the user in this case)

* allow to overwrite `.config` if `--force` (or an equivalent "force" option is passed)

* no interactive questions"

It sounds good to me. Technically, would it then be OK to have these options:

• --create (or similar): do not overwrite .config if it already exists and warn the user in this case
• --update: we keep the previous behaviour (from virtme): the .config is updated with the required config. Maybe it is just me, but I would expect the --update option to update an existing config (and complain if there is no .config?), no need to use a --force option, no?

[matttbe]

--create (or similar): do not overwrite .config if it already exists and warn the user in this case

I added a --no-update option in my last version.

Please note that if the .config file is present, a message will be printed to stdout: in other words, this will not be visible if vng is not launched with --verbose. Should it be printed to stderr instead?

[arighi]

--create (or similar): do not overwrite .config if it already exists and warn the user in this case

I added a --no-update option in my last version.

Please note that if the .config file is present, a message will be printed to stdout: in other words, this will not be visible if vng is not launched with --verbose. Should it be printed to stderr instead?

I think it's ok to print the message to stdout, it's supposed to be the expected behavior.

Maybe we could pass --no-update to virtme-configkernel unless --force is specified in vng, so if we really want to update the .config we can do something like vng --kconfig --force.

Something like this:

diff --git a/virtme_ng/run.py b/virtme_ng/run.py
index 0e88db4..c6b0468 100644
--- a/virtme_ng/run.py
+++ b/virtme_ng/run.py
@@ -565,7 +565,9 @@ class KernelSource:
     def config(self, args):
         """Perform a make config operation on a kernel source directory."""
         arch = args.arch
-        cmd = "virtme-configkernel --defconfig --no-update"
+        cmd = "virtme-configkernel --defconfig"
+        if not args.force:
+            cmd += " --no-update"
         if arch is not None:
             if arch not in ARCH_MAPPING:
                 arg_fail(f"unsupported architecture: {arch}")

What do you think?

Anyway, this looks good to me and we can work on other improvements in tree. Thanks!

[matttbe]

--create (or similar): do not overwrite .config if it already exists and warn the user in this case

I added a --no-update option in my last version.
Please note that if the .config file is present, a message will be printed to stdout: in other words, this will not be visible if vng is not launched with --verbose. Should it be printed to stderr instead?

I think it's ok to print the message to stdout, it's supposed to be the expected behavior.

👍

Maybe we could pass --no-update to virtme-configkernel unless --force is specified in vng, so if we really want to update the .config we can do something like vng --kconfig --force.

Good idea! Sorry, I didn't get before you were talking about vng's --force option.

Something like this:

diff --git a/virtme_ng/run.py b/virtme_ng/run.py
index 0e88db4..c6b0468 100644
--- a/virtme_ng/run.py
+++ b/virtme_ng/run.py
@@ -565,7 +565,9 @@ class KernelSource:
     def config(self, args):
         """Perform a make config operation on a kernel source directory."""
         arch = args.arch
-        cmd = "virtme-configkernel --defconfig --no-update"
+        cmd = "virtme-configkernel --defconfig"
+        if not args.force:
+            cmd += " --no-update"
         if arch is not None:
             if arch not in ARCH_MAPPING:
                 arg_fail(f"unsupported architecture: {arch}")

What do you think?

Yes, it looks good to me! If you did the modification, I guess you will commit it? Or do you prefer if I open a new PR?

Anyway, this looks good to me and we can work on other improvements in tree. Thanks!

Thanks!

[arighi]

@matttbe feel free to create a new PR for that. Otherwise I'll create it later today.