[Buildroot] [PATCH 1/1] support/scripts/: remove -E flag from patch call

[Buildroot] [PATCH 1/1] support/scripts/: remove -E flag from patch call

[Andrey Nechypurenko]

-E flag instructs patch to remove empty files. However, in some cases
empty files are essential. If they are missing, build could be broken
or other bad things can happen.

Signed-off-by: Andrey Nechypurenko <andreynech@gmail.com>
---
 support/scripts/apply-patches.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
index e5a2fdd09e..6da83f6826 100755
--- a/support/scripts/apply-patches.sh
+++ b/support/scripts/apply-patches.sh
@@ -114,7 +114,7 @@ function apply_patch {
         exit 1
     fi
     echo "${path}/${patch}" >> ${builddir}/.applied_patches_list
-    ${uncomp} "${path}/$patch" | patch -g0 -p1 -E --no-backup-if-mismatch -d "${builddir}" -t -N $silent
+    ${uncomp} "${path}/$patch" | patch -g0 -p1 --no-backup-if-mismatch -d "${builddir}" -t -N $silent
     if [ $? != 0 ] ; then
         echo "Patch failed!  Please fix ${patch}!"
         exit 1
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] support/scripts/: remove -E flag from patch call

[Yann E. MORIN]

Andrey, All,

On 2021-12-14 13:55 +0100, Andrey Nechypurenko spake thusly:
> -E flag instructs patch to remove empty files. However, in some cases
> empty files are essential. If they are missing, build could be broken
> or other bad things can happen.

Do you have an example of a publicly accessible package that has this
issue?

> Signed-off-by: Andrey Nechypurenko <andreynech@gmail.com>

I was going to apply this, but then I was wondering; what about patches
that actually *want* to remove files?

It turns out that we do have a few patches that remove files:

    $ git grep -E '\+\+\+ /dev/null'
    board/roseapplepi/patches/uboot/0001-compiler-.h-sync-include-linux-compiler-.h-with-Linu.patch:+++ /dev/null
    board/roseapplepi/patches/uboot/0001-compiler-.h-sync-include-linux-compiler-.h-with-Linu.patch:+++ /dev/null
    board/roseapplepi/patches/uboot/0001-compiler-.h-sync-include-linux-compiler-.h-with-Linu.patch:+++ /dev/null
    boot/grub2/0033-verifiers-Move-verifiers-API-to-kernel-image.patch:+++ /dev/null
    boot/grub2/0034-efi-Move-the-shim_lock-verifier-to-the-GRUB-core.patch:+++ /dev/null
    boot/lpc32xxcdl/0002-delete_redundant_files.patch:+++ /dev/null2012-01-01 16:39:47.918907000 +0100
    boot/lpc32xxcdl/0002-delete_redundant_files.patch:+++ /dev/null2012-01-01 16:39:47.918907000 +0100
    package/babeltrace2/0001-configure-simplify-warning-flags-detection.patch:+++ /dev/null
    package/babeltrace2/0001-configure-simplify-warning-flags-detection.patch:+++ /dev/null
    package/babeltrace2/0001-configure-simplify-warning-flags-detection.patch:+++ /dev/null
    package/babeltrace2/0001-configure-simplify-warning-flags-detection.patch:+++ /dev/null
    package/gcc/10.3.0/0001-Revert-re-PR-target-92095-internal-error-with-O1-mcp.patch:+++ /dev/null
    package/gcc/8.4.0/0002-Revert-re-PR-target-92095-internal-error-with-O1-mcp.patch:+++ /dev/null
    package/gcc/9.4.0/0003-Revert-re-PR-target-92095-internal-error-with-O1-mcp.patch:+++ /dev/null
    package/openpgm/0001-Rename-openpgm-5.2.pc.in.patch:+++ /dev/null
    package/openvmtools/0003-Rename-poll-h-into-vm_poll-h-to-fix-build-failure-on-musl.patch:+++ /dev/null
    package/screen/0005-rename-sched_h.patch:+++ /dev/null

While a few of them could be rewriten so that the are actual renames
(now that we require a patch version that supports renames), some really
do want to remove files; that's notably the case with the uboot patch
for the roseapplepi board, or the babeltrace2 patch, as well as the gcc
patches...

Furthermore, some users may have alaready relied on this behaviour, that
empty files get removed, for their packages in theor br2-external
trees...

So we have an antagonist situation: some patches will want to rem ove
files, and some patches will want to not remove files. We can't satisfy
both, obviously.

So, unfortunately, it is not possible to apply your patch.

If your package really requires that the files be present and empty, you
should then use a post-patch hook to touch those files.

Unless someone comes up with a better idea...

Regards,
Yann E. MORIN.

> ---
>  support/scripts/apply-patches.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
> index e5a2fdd09e..6da83f6826 100755
> --- a/support/scripts/apply-patches.sh
> +++ b/support/scripts/apply-patches.sh
> @@ -114,7 +114,7 @@ function apply_patch {
>          exit 1
>      fi
>      echo "${path}/${patch}" >> ${builddir}/.applied_patches_list
> -    ${uncomp} "${path}/$patch" | patch -g0 -p1 -E --no-backup-if-mismatch -d "${builddir}" -t -N $silent
> +    ${uncomp} "${path}/$patch" | patch -g0 -p1 --no-backup-if-mismatch -d "${builddir}" -t -N $silent
>      if [ $? != 0 ] ; then
>          echo "Patch failed!  Please fix ${patch}!"
>          exit 1
> -- 
> 2.25.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] support/scripts/: remove -E flag from patch call

[Andrey Nechypurenko]

Hi Yann,

>> -E flag instructs patch to remove empty files. However, in some cases
>> empty files are essential. If they are missing, build could be broken
>> or other bad things can happen.
>
> Do you have an example of a publicly accessible package that has this
> issue?

No I have not. In my case this is not a mainlined driver for the touch
controller provided by the display manufacturer.

> So we have an antagonist situation: some patches will want to rem ove
> files, and some patches will want to not remove files. We can't satisfy
> both, obviously.
>
> So, unfortunately, it is not possible to apply your patch.

I understand your concern. So it sounds like a need for configurability
here.

> Unless someone comes up with a better idea...

Not sure if it is a good idea, but maybe the problem could be solved using
series files? Currently, there is already -p1 parameter which could be
specified in the series files. Brief look at the relevant part of the
apply-patches.sh reveals that it is ignored :-) but it could be changed.
After that, the script can walk through patch directories where patches with
+++ /dev/null are found, generate or update series files to use the -E flag
together with -p1. This solution would not break existing packages and
provide flexibility for the new packages where developers can decide whether
to add -E (or maybe some other flags) to patch invocation using series
files.

What do you think about it?

Best regards,
Andrey.
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] support/scripts/: remove -E flag from patch call

[Arnout Vandecappelle]

On 30/12/2021 22:47, Yann E. MORIN wrote:
> Andrey, All,
> 
> On 2021-12-14 13:55 +0100, Andrey Nechypurenko spake thusly:
>> -E flag instructs patch to remove empty files. However, in some cases
>> empty files are essential. If they are missing, build could be broken
>> or other bad things can happen.
> 
> Do you have an example of a publicly accessible package that has this
> issue?
> 
>> Signed-off-by: Andrey Nechypurenko <andreynech@gmail.com>
> 
> I was going to apply this, but then I was wondering; what about patches
> that actually *want* to remove files?
> 
> It turns out that we do have a few patches that remove files:
> 
>      $ git grep -E '\+\+\+ /dev/null'
>      board/roseapplepi/patches/uboot/0001-compiler-.h-sync-include-linux-compiler-.h-with-Linu.patch:+++ /dev/null
>      board/roseapplepi/patches/uboot/0001-compiler-.h-sync-include-linux-compiler-.h-with-Linu.patch:+++ /dev/null
>      board/roseapplepi/patches/uboot/0001-compiler-.h-sync-include-linux-compiler-.h-with-Linu.patch:+++ /dev/null
>      boot/grub2/0033-verifiers-Move-verifiers-API-to-kernel-image.patch:+++ /dev/null
>      boot/grub2/0034-efi-Move-the-shim_lock-verifier-to-the-GRUB-core.patch:+++ /dev/null
>      boot/lpc32xxcdl/0002-delete_redundant_files.patch:+++ /dev/null2012-01-01 16:39:47.918907000 +0100
>      boot/lpc32xxcdl/0002-delete_redundant_files.patch:+++ /dev/null2012-01-01 16:39:47.918907000 +0100
>      package/babeltrace2/0001-configure-simplify-warning-flags-detection.patch:+++ /dev/null
>      package/babeltrace2/0001-configure-simplify-warning-flags-detection.patch:+++ /dev/null
>      package/babeltrace2/0001-configure-simplify-warning-flags-detection.patch:+++ /dev/null
>      package/babeltrace2/0001-configure-simplify-warning-flags-detection.patch:+++ /dev/null

  Have you actually tested this? With modern patch, the /dev/null indicates 
exactly what we want: that the file must be removed. So even without -E, the 
file gets removed.

  What -E does is remove a file in the following case:

--- a/foo
+++ a/foo
@@ -1,1 +0,0 @@
- foo


git grep -B1 '+0,0 @@' only yields /dev/null cases, so I think it's OK.


  So I think the only real concern is: will a patch version that supports 
renames always behave like this? I *think* the answer is yes, but it's hard to 
be sure of course.

  Regards,
  Arnout



>      package/gcc/10.3.0/0001-Revert-re-PR-target-92095-internal-error-with-O1-mcp.patch:+++ /dev/null
>      package/gcc/8.4.0/0002-Revert-re-PR-target-92095-internal-error-with-O1-mcp.patch:+++ /dev/null
>      package/gcc/9.4.0/0003-Revert-re-PR-target-92095-internal-error-with-O1-mcp.patch:+++ /dev/null
>      package/openpgm/0001-Rename-openpgm-5.2.pc.in.patch:+++ /dev/null
>      package/openvmtools/0003-Rename-poll-h-into-vm_poll-h-to-fix-build-failure-on-musl.patch:+++ /dev/null
>      package/screen/0005-rename-sched_h.patch:+++ /dev/null
> 
> While a few of them could be rewriten so that the are actual renames
> (now that we require a patch version that supports renames), some really
> do want to remove files; that's notably the case with the uboot patch
> for the roseapplepi board, or the babeltrace2 patch, as well as the gcc
> patches...
> 
> Furthermore, some users may have alaready relied on this behaviour, that
> empty files get removed, for their packages in theor br2-external
> trees...
> 
> So we have an antagonist situation: some patches will want to rem ove
> files, and some patches will want to not remove files. We can't satisfy
> both, obviously.
> 
> So, unfortunately, it is not possible to apply your patch.
> 
> If your package really requires that the files be present and empty, you
> should then use a post-patch hook to touch those files.
> 
> Unless someone comes up with a better idea...
> 
> Regards,
> Yann E. MORIN.
> 
>> ---
>>   support/scripts/apply-patches.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
>> index e5a2fdd09e..6da83f6826 100755
>> --- a/support/scripts/apply-patches.sh
>> +++ b/support/scripts/apply-patches.sh
>> @@ -114,7 +114,7 @@ function apply_patch {
>>           exit 1
>>       fi
>>       echo "${path}/${patch}" >> ${builddir}/.applied_patches_list
>> -    ${uncomp} "${path}/$patch" | patch -g0 -p1 -E --no-backup-if-mismatch -d "${builddir}" -t -N $silent
>> +    ${uncomp} "${path}/$patch" | patch -g0 -p1 --no-backup-if-mismatch -d "${builddir}" -t -N $silent
>>       if [ $? != 0 ] ; then
>>           echo "Patch failed!  Please fix ${patch}!"
>>           exit 1
>> -- 
>> 2.25.1
>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot@buildroot.org
>> https://lists.buildroot.org/mailman/listinfo/buildroot
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] support/scripts/: remove -E flag from patch call

[Yann E. MORIN]

Arnout, All,

On 2022-01-03 18:11 +0100, Arnout Vandecappelle spake thusly:
> On 30/12/2021 22:47, Yann E. MORIN wrote:
> >On 2021-12-14 13:55 +0100, Andrey Nechypurenko spake thusly:
> >>-E flag instructs patch to remove empty files. However, in some cases
> >>empty files are essential. If they are missing, build could be broken
> >>or other bad things can happen.
[--SNIP--]
> >I was going to apply this, but then I was wondering; what about patches
> >that actually *want* to remove files?
> >It turns out that we do have a few patches that remove files:
>  Have you actually tested this? With modern patch, the /dev/null indicates
> exactly what we want: that the file must be removed. So even without -E, the
> file gets removed.
> 
>  What -E does is remove a file in the following case:
> 
> --- a/foo
> +++ a/foo
> @@ -1,1 +0,0 @@
> - foo

Ah, yeah, I was a bit unsure... Now I tried, adn indeed it behaves as
expected.

> git grep -B1 '+0,0 @@' only yields /dev/null cases, so I think it's OK.
>  So I think the only real concern is: will a patch version that supports
> renames always behave like this? I *think* the answer is yes, but it's hard
> to be sure of course.

OK, I'm sold on the idea, and I believe this is sane enough.

Applied to master, thanks.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot