Yocto rpm packages do not keep the modified CONFFILES (was: BB Equivalent for %config(noreplace))

Hi,

I just found out that if an updated package ships a modified
configuration file, then the Yocto package upgrade does this:

* rename the edited conffile to conffile.rpmsave
* install the new, modified one from the package

instead of:

* keep the edited conffile intact
* install the new, modified one from the package as conffile.rpmnew

I think this is what was asked in 2017 here:
https://docs.yoctoproject.org/pipermail/yocto/2017-May/036031.html

The answer was a little uninformed about saying that using
CONFFILES is the fix, because rpm can behave two ways, see above.

Yocto, to this day, uses "%config /path/to/conffile" instead of
"%config(noreplace) /path/to/conffile" in package_rpm.bbclass.
The latter would behave as "keep modified conffile + install conffile.rpmnew".

What's the official stance on this behaviour?
Is it a known issue?
Is this what the project would expect from a package upgrade?

FWIW, this one liner would change the behaviour:

          # packages-split/ and not package/
@@ -250,13 +250,19 @@ python write_specfile () {
                  if (not files and not dirs) or path in dirfiles:
                      target.append(attr + '%dir "' + escape_chars(path) + '"')

+            conffiles_mode = d.getVar("CONFFILES_MODE") or "installnew"
+            if conffiles_mode != "installnew" and conffiles_mode != "keepold":
+                conffiles_mode = "installnew"
              for file in files:
                  if file == "CONTROL" or file == "DEBIAN":
                      continue
                  attr = get_attr(file)
                  p = path + '/' + file
                  if conffiles.count(p):
-                    target.append(attr + '%config "' + escape_chars(p) + '"')
+                    if conffiles_mode == "keepold":
+                        target.append(attr + '%config(noreplace) "' + escape_chars(p) + '"')
+                    else:
+                        target.append(attr + '%config "' + escape_chars(p) + '"')
                  else:
                      target.append(attr + '"' + escape_chars(p) + '"')

@@ -430,7 +436,7 @@ python write_specfile () {
              srcrpostrm     = splitrpostrm

              file_list = []
-            walk_files(root, file_list, conffiles, dirfiles)
+            walk_files(localdata, root, file_list, conffiles, dirfiles)
              if not file_list and localdata.getVar('ALLOW_EMPTY', False) != "1":
                  bb.note("Not creating empty RPM package for %s" % splitname)
              else:
@@ -522,7 +528,7 @@ python write_specfile () {

          # Now process files
          file_list = []
-        walk_files(root, file_list, conffiles, dirfiles)
+        walk_files(localdata, root, file_list, conffiles, dirfiles)
          if not file_list and localdata.getVar('ALLOW_EMPTY', False) != "1":
              bb.note("Not creating empty RPM package for %s" % splitname)
          else:

Best regards,
Zoltán Böszörményi

Thank you for your submission. Patchtest identified one
or more issues with the patch. Please see the log below for
more information:

---
Testing patch /home/patchtest/share/mboxes/Yocto-rpm-packages-do-not-keep-the-modified-CONFFILES-was-BB-Equivalent-for-config-noreplace.patch

FAIL: test Signed-off-by presence: Mbox is missing Signed-off-by. Add it manually or with "git commit --amend -s" (test_mbox.TestMbox.test_signed_off_by_presence)
FAIL: test mbox format: Series has malformed diff lines. Create the series again using git-format-patch and ensure it applies using git am (test_mbox.TestMbox.test_mbox_format)
FAIL: test shortlog length: Edit shortlog so that it is 90 characters or less (currently 97 characters) (test_mbox.TestMbox.test_shortlog_length)

PASS: test author valid (test_mbox.TestMbox.test_author_valid)
PASS: test commit message presence (test_mbox.TestMbox.test_commit_message_presence)
PASS: test max line length (test_metadata.TestMetadata.test_max_line_length)
PASS: test non-AUH upgrade (test_mbox.TestMbox.test_non_auh_upgrade)
PASS: test shortlog format (test_mbox.TestMbox.test_shortlog_format)
PASS: test target mailing list (test_mbox.TestMbox.test_target_mailing_list)

SKIP: pretest pylint: Python-unidiff parse error (test_python_pylint.PyLint.pretest_pylint)
SKIP: pretest src uri left files: Patch cannot be merged (test_metadata.TestMetadata.pretest_src_uri_left_files)
SKIP: test CVE check ignore: No modified recipes or older target branch, skipping test (test_metadata.TestMetadata.test_cve_check_ignore)
SKIP: test CVE tag format: Parse error Unexpected hunk found: @@ -250,13 +250,19 @@ python write_specfile () {
SKIP: test Signed-off-by presence: Parse error Unexpected hunk found: @@ -250,13 +250,19 @@ python write_specfile () {
SKIP: test Upstream-Status presence: Parse error Unexpected hunk found: @@ -250,13 +250,19 @@ python write_specfile () {
SKIP: test bugzilla entry format: No bug ID found (test_mbox.TestMbox.test_bugzilla_entry_format)
SKIP: test lic files chksum modified not mentioned: No modified recipes, skipping test (test_metadata.TestMetadata.test_lic_files_chksum_modified_not_mentioned)
SKIP: test lic files chksum presence: No added recipes, skipping test (test_metadata.TestMetadata.test_lic_files_chksum_presence)
SKIP: test license presence: No added recipes, skipping test (test_metadata.TestMetadata.test_license_presence)
SKIP: test pylint: Python-unidiff parse error (test_python_pylint.PyLint.test_pylint)
SKIP: test series merge on head: Merge test is disabled for now (test_mbox.TestMbox.test_series_merge_on_head)
SKIP: test src uri left files: Patch cannot be merged (test_metadata.TestMetadata.test_src_uri_left_files)
SKIP: test summary presence: No added recipes, skipping test (test_metadata.TestMetadata.test_summary_presence)

---

Please address the issues identified and
submit a new revision of the patch, or alternatively, reply to this
email with an explanation of why the patch should be accepted. If you
believe these results are due to an error in patchtest, please submit a
bug at https://bugzilla.yoctoproject.org/ (use the 'Patchtest' category
under 'Yocto Project Subprojects'). For more information on specific
failures, see: https://wiki.yoctoproject.org/wiki/Patchtest. Thank
you!

On Fri, 2024-03-08 at 03:20 -0800, Zoltan Boszormenyi wrote:
> Hi,
> 
> I just found out that if an updated package ships a modified
> configuration file, then the Yocto package upgrade does this:
> 
> * rename the edited conffile to conffile.rpmsave
> * install the new, modified one from the package
> 
> instead of:
> 
> * keep the edited conffile intact
> * install the new, modified one from the package as conffile.rpmnew
> 
> I think this is what was asked in 2017 here:
> https://docs.yoctoproject.org/pipermail/yocto/2017-May/036031.html
> 
> The answer was a little uninformed about saying that using
> CONFFILES is the fix, because rpm can behave two ways, see above.
> 
> Yocto, to this day, uses "%config /path/to/conffile" instead of
> "%config(noreplace) /path/to/conffile" in package_rpm.bbclass.
> The latter would behave as "keep modified conffile + install
> conffile.rpmnew".
> 
> What's the official stance on this behaviour?
> Is it a known issue?
> Is this what the project would expect from a package upgrade?

I would be interested to know what the ipk and deb backends do...

In some ways, I'd also like to have a test case and then we can define
and document what the correct behaviour is too.

Cheers,

Richard

I think (noreplace) flag only matters when the file has been manually
edited after rpm installation and its checksum diverges from what is
registered in rpm database. Which is not a common scenario in
embedded. If you manually edit the file on target, you might as well
manually resolve which version you want after the update as both are
available regardless of whether (noreplace) was used or not. Or
perhaps merge the manual edits into the new version.

Alex

On Fri, 8 Mar 2024 at 12:20, Zoltan Boszormenyi <zboszor@gmail.com> wrote:
>
> Hi,
>
> I just found out that if an updated package ships a modified
> configuration file, then the Yocto package upgrade does this:
>
> * rename the edited conffile to conffile.rpmsave
> * install the new, modified one from the package
>
> instead of:
>
> * keep the edited conffile intact
> * install the new, modified one from the package as conffile.rpmnew
>
> I think this is what was asked in 2017 here:
> https://docs.yoctoproject.org/pipermail/yocto/2017-May/036031.html
>
> The answer was a little uninformed about saying that using
> CONFFILES is the fix, because rpm can behave two ways, see above.
>
> Yocto, to this day, uses "%config /path/to/conffile" instead of
> "%config(noreplace) /path/to/conffile" in package_rpm.bbclass.
> The latter would behave as "keep modified conffile + install conffile.rpmnew".
>
> What's the official stance on this behaviour?
> Is it a known issue?
> Is this what the project would expect from a package upgrade?
>
> FWIW, this one liner would change the behaviour:
>
> diff --git a/meta/classes-global/package_rpm.bbclass b/meta/classes-global/package_rpm.bbclass
> index 2e3e4e8c79..6610115849 100644
> --- a/meta/classes-global/package_rpm.bbclass
> +++ b/meta/classes-global/package_rpm.bbclass
> @@ -256,7 +256,7 @@ python write_specfile () {
>                   attr = get_attr(file)
>                   p = path + '/' + file
>                   if conffiles.count(p):
> -                    target.append(attr + '%config "' + escape_chars(p) + '"')
> +                    target.append(attr + '%config(noreplace) "' + escape_chars(p) + '"')
>                   else:
>                       target.append(attr + '"' + escape_chars(p) + '"')
>
>
> Maybe it's better to make it conditional on a different variable:
>
> CONFFILES_MODE = "keepold"
> vs
> CONFFILES_MODE = "installnew"
>
> and the patch would be something like this below (untested),
> keeping the current behaviour as default:
>
> diff --git a/meta/classes-global/package_rpm.bbclass b/meta/classes-global/package_rpm.bbclass
> index 2e3e4e8c79..f0651d69ba 100644
> --- a/meta/classes-global/package_rpm.bbclass
> +++ b/meta/classes-global/package_rpm.bbclass
> @@ -191,7 +191,7 @@ python write_specfile () {
>                   if not len(depends_dict[dep]):
>                       array.append("%s: %s" % (tag, dep))
>
> -    def walk_files(walkpath, target, conffiles, dirfiles):
> +    def walk_files(d, walkpath, target, conffiles, dirfiles):
>           # We can race against the ipk/deb backends which create CONTROL or DEBIAN
> directories
>           # when packaging. We just ignore these files which are created in
>           # packages-split/ and not package/
> @@ -250,13 +250,19 @@ python write_specfile () {
>                   if (not files and not dirs) or path in dirfiles:
>                       target.append(attr + '%dir "' + escape_chars(path) + '"')
>
> +            conffiles_mode = d.getVar("CONFFILES_MODE") or "installnew"
> +            if conffiles_mode != "installnew" and conffiles_mode != "keepold":
> +                conffiles_mode = "installnew"
>               for file in files:
>                   if file == "CONTROL" or file == "DEBIAN":
>                       continue
>                   attr = get_attr(file)
>                   p = path + '/' + file
>                   if conffiles.count(p):
> -                    target.append(attr + '%config "' + escape_chars(p) + '"')
> +                    if conffiles_mode == "keepold":
> +                        target.append(attr + '%config(noreplace) "' + escape_chars(p) + '"')
> +                    else:
> +                        target.append(attr + '%config "' + escape_chars(p) + '"')
>                   else:
>                       target.append(attr + '"' + escape_chars(p) + '"')
>
> @@ -430,7 +436,7 @@ python write_specfile () {
>               srcrpostrm     = splitrpostrm
>
>               file_list = []
> -            walk_files(root, file_list, conffiles, dirfiles)
> +            walk_files(localdata, root, file_list, conffiles, dirfiles)
>               if not file_list and localdata.getVar('ALLOW_EMPTY', False) != "1":
>                   bb.note("Not creating empty RPM package for %s" % splitname)
>               else:
> @@ -522,7 +528,7 @@ python write_specfile () {
>
>           # Now process files
>           file_list = []
> -        walk_files(root, file_list, conffiles, dirfiles)
> +        walk_files(localdata, root, file_list, conffiles, dirfiles)
>           if not file_list and localdata.getVar('ALLOW_EMPTY', False) != "1":
>               bb.note("Not creating empty RPM package for %s" % splitname)
>           else:
>
> Best regards,
> Zoltán Böszörményi
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#196848): https://lists.openembedded.org/g/openembedded-core/message/196848
> Mute This Topic: https://lists.openembedded.org/mt/104806483/1686489
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [alex.kanavin@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>

2024. 03. 09. 17:39 keltezéssel, Alexander Kanavin írta:
> I think (noreplace) flag only matters when the file has been manually
> edited after rpm installation and its checksum diverges from what is
> registered in rpm database. Which is not a common scenario in
> embedded.

So from Yocto's POV it's the same either way, right?

>   If you manually edit the file on target, you might as well
> manually resolve which version you want after the update as both are
> available regardless of whether (noreplace) was used or not. Or
> perhaps merge the manual edits into the new version.

The interesting use case here is that the configuration file
in question is generated on the machine using a GUI,
while a minimal stock version of the file is shipped by the package.

>
> Alex
>
> On Fri, 8 Mar 2024 at 12:20, Zoltan Boszormenyi <zboszor@gmail.com> wrote:
>> Hi,
>>
>> I just found out that if an updated package ships a modified
>> configuration file, then the Yocto package upgrade does this:
>>
>> * rename the edited conffile to conffile.rpmsave
>> * install the new, modified one from the package
>>
>> instead of:
>>
>> * keep the edited conffile intact
>> * install the new, modified one from the package as conffile.rpmnew
>>
>> I think this is what was asked in 2017 here:
>> https://docs.yoctoproject.org/pipermail/yocto/2017-May/036031.html
>>
>> The answer was a little uninformed about saying that using
>> CONFFILES is the fix, because rpm can behave two ways, see above.
>>
>> Yocto, to this day, uses "%config /path/to/conffile" instead of
>> "%config(noreplace) /path/to/conffile" in package_rpm.bbclass.
>> The latter would behave as "keep modified conffile + install conffile.rpmnew".
>>
>> What's the official stance on this behaviour?
>> Is it a known issue?
>> Is this what the project would expect from a package upgrade?
>>
>> FWIW, this one liner would change the behaviour:
>>
>> diff --git a/meta/classes-global/package_rpm.bbclass b/meta/classes-global/package_rpm.bbclass
>> index 2e3e4e8c79..6610115849 100644
>> --- a/meta/classes-global/package_rpm.bbclass
>> +++ b/meta/classes-global/package_rpm.bbclass
>> @@ -256,7 +256,7 @@ python write_specfile () {
>>                    attr = get_attr(file)
>>                    p = path + '/' + file
>>                    if conffiles.count(p):
>> -                    target.append(attr + '%config "' + escape_chars(p) + '"')
>> +                    target.append(attr + '%config(noreplace) "' + escape_chars(p) + '"')
>>                    else:
>>                        target.append(attr + '"' + escape_chars(p) + '"')
>>
>>
>> Maybe it's better to make it conditional on a different variable:
>>
>> CONFFILES_MODE = "keepold"
>> vs
>> CONFFILES_MODE = "installnew"
>>
>> and the patch would be something like this below (untested),
>> keeping the current behaviour as default:
>>
>> diff --git a/meta/classes-global/package_rpm.bbclass b/meta/classes-global/package_rpm.bbclass
>> index 2e3e4e8c79..f0651d69ba 100644
>> --- a/meta/classes-global/package_rpm.bbclass
>> +++ b/meta/classes-global/package_rpm.bbclass
>> @@ -191,7 +191,7 @@ python write_specfile () {
>>                    if not len(depends_dict[dep]):
>>                        array.append("%s: %s" % (tag, dep))
>>
>> -    def walk_files(walkpath, target, conffiles, dirfiles):
>> +    def walk_files(d, walkpath, target, conffiles, dirfiles):
>>            # We can race against the ipk/deb backends which create CONTROL or DEBIAN
>> directories
>>            # when packaging. We just ignore these files which are created in
>>            # packages-split/ and not package/
>> @@ -250,13 +250,19 @@ python write_specfile () {
>>                    if (not files and not dirs) or path in dirfiles:
>>                        target.append(attr + '%dir "' + escape_chars(path) + '"')
>>
>> +            conffiles_mode = d.getVar("CONFFILES_MODE") or "installnew"
>> +            if conffiles_mode != "installnew" and conffiles_mode != "keepold":
>> +                conffiles_mode = "installnew"
>>                for file in files:
>>                    if file == "CONTROL" or file == "DEBIAN":
>>                        continue
>>                    attr = get_attr(file)
>>                    p = path + '/' + file
>>                    if conffiles.count(p):
>> -                    target.append(attr + '%config "' + escape_chars(p) + '"')
>> +                    if conffiles_mode == "keepold":
>> +                        target.append(attr + '%config(noreplace) "' + escape_chars(p) + '"')
>> +                    else:
>> +                        target.append(attr + '%config "' + escape_chars(p) + '"')
>>                    else:
>>                        target.append(attr + '"' + escape_chars(p) + '"')
>>
>> @@ -430,7 +436,7 @@ python write_specfile () {
>>                srcrpostrm     = splitrpostrm
>>
>>                file_list = []
>> -            walk_files(root, file_list, conffiles, dirfiles)
>> +            walk_files(localdata, root, file_list, conffiles, dirfiles)
>>                if not file_list and localdata.getVar('ALLOW_EMPTY', False) != "1":
>>                    bb.note("Not creating empty RPM package for %s" % splitname)
>>                else:
>> @@ -522,7 +528,7 @@ python write_specfile () {
>>
>>            # Now process files
>>            file_list = []
>> -        walk_files(root, file_list, conffiles, dirfiles)
>> +        walk_files(localdata, root, file_list, conffiles, dirfiles)
>>            if not file_list and localdata.getVar('ALLOW_EMPTY', False) != "1":
>>                bb.note("Not creating empty RPM package for %s" % splitname)
>>            else:
>>
>> Best regards,
>> Zoltán Böszörményi
>>
>>
>>
>>
>>
>> -=-=-=-=-=-=-=-=-=-=-=-
>> Links: You receive all messages sent to this group.
>> View/Reply Online (#196869): https://lists.openembedded.org/g/openembedded-core/message/196869
>> Mute This Topic: https://lists.openembedded.org/mt/104806483/3617728
>> Group Owner: openembedded-core+owner@lists.openembedded.org
>> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [zboszor@gmail.com]
>> -=-=-=-=-=-=-=-=-=-=-=-
>>

On Mon, 11 Mar 2024 at 10:30, Böszörményi Zoltán <zboszor@gmail.com> wrote:

> >   If you manually edit the file on target, you might as well
> > manually resolve which version you want after the update as both are
> > available regardless of whether (noreplace) was used or not. Or
> > perhaps merge the manual edits into the new version.
>
> The interesting use case here is that the configuration file
> in question is generated on the machine using a GUI,
> while a minimal stock version of the file is shipped by the package.

This can be handled by shipping the stock config under a different
file name (e.g. conf.sample or conf.template)?

Alex

2024. 03. 11. 10:37 keltezéssel, Alexander Kanavin írta:
> On Mon, 11 Mar 2024 at 10:30, Böszörményi Zoltán <zboszor@gmail.com> wrote:
>
>>>    If you manually edit the file on target, you might as well
>>> manually resolve which version you want after the update as both are
>>> available regardless of whether (noreplace) was used or not. Or
>>> perhaps merge the manual edits into the new version.
>> The interesting use case here is that the configuration file
>> in question is generated on the machine using a GUI,
>> while a minimal stock version of the file is shipped by the package.
> This can be handled by shipping the stock config under a different
> file name (e.g. conf.sample or conf.template)?

Yes, I am aware. But why not use the package manager's
own features instead of adding a workaround?

On Mon, 11 Mar 2024 at 11:09, Böszörményi Zoltán <zboszor@gmail.com> wrote:
> Yes, I am aware. But why not use the package manager's
> own features instead of adding a workaround?

Because I see it the opposite way: noreplace in this case is a
workaround for a file name conflict between what the tool generates
and what the package installs. If you're going to generate a file on
target, then you shouldn't be installing it from a package.

Alex

Packages + overrides are a very common way to configure/customize images.

Take OE itself as an example, the sshd_config is a conffile for openssh, but in rootfs-postcommands.bbclass, it is customized.
This means sshd_config might be different from the one that is recorded in rpm database. This is a similar situation with the original question.
The original question is basically equal to: are we going to keep the sshd_config we customized when we upgrade the openssh on target?

Ideally, packages should be designed to allow config snippets (/etc/xxx.conf.d/) or some override mechanism (default conf in /usr/lib and override in /etc). But there'll always be packages that lack such mechanisms. Using 'noreplace' seems a reasonable choice.

For the default behavior of OE, I'd suggest we change to use 'noreplace'. Because the more unlikely people modify their target files (as Alex pointed out for embedded devices), the more important those modifications might be.

Regards,
Qi

-----Original Message-----
From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> On Behalf Of Alexander Kanavin
Sent: Monday, March 11, 2024 6:20 PM
To: Böszörményi Zoltán <zboszor@gmail.com>
Cc: openembedded-core@lists.openembedded.org
Subject: Re: [OE-core] Yocto rpm packages do not keep the modified CONFFILES

On Mon, 11 Mar 2024 at 11:09, Böszörményi Zoltán <zboszor@gmail.com> wrote:
> Yes, I am aware. But why not use the package manager's own features 
> instead of adding a workaround?

Because I see it the opposite way: noreplace in this case is a workaround for a file name conflict between what the tool generates and what the package installs. If you're going to generate a file on target, then you shouldn't be installing it from a package.

Alex

On Tue, 12 Mar 2024 at 02:57, Chen, Qi <Qi.Chen@windriver.com> wrote:
>
> Packages + overrides are a very common way to configure/customize images.
>
> Take OE itself as an example, the sshd_config is a conffile for openssh, but in rootfs-postcommands.bbclass, it is customized.
> This means sshd_config might be different from the one that is recorded in rpm database. This is a similar situation with the original question.
> The original question is basically equal to: are we going to keep the sshd_config we customized when we upgrade the openssh on target?
>
> Ideally, packages should be designed to allow config snippets (/etc/xxx.conf.d/) or some override mechanism (default conf in /usr/lib and override in /etc). But there'll always be packages that lack such mechanisms. Using 'noreplace' seems a reasonable choice.
>
> For the default behavior of OE, I'd suggest we change to use 'noreplace'. Because the more unlikely people modify their target files (as Alex pointed out for embedded devices), the more important those modifications might be.

With this I agree. There's also just been a patchset that refactors
ssh config into just that kind of snippet mechanism.

But if we add noreplace, we also need to ensure its behaviour is
consistent with deb and ipk backends as RP said.

Alex