diff mbox series

[3/4] kbuild: slim down package for building external modules

Message ID 20240727074526.1771247-4-masahiroy@kernel.org (mailing list archive)
State New, archived
Headers show
Series kbuild: cross-compile linux-headers package | expand

Commit Message

Masahiro Yamada July 27, 2024, 7:42 a.m. UTC
Exclude directories and files unnecessary for building external modules:

 - include/config/  (except include/config/auto.conf)
 - scripts/atomic/
 - scripts/dtc/
 - scripts/kconfig/
 - scripts/mod/mk_elfconfig
 - scripts/package/
 - scripts/unifdef
 - .config
 - *.o
 - .*.cmd

Avoid copying files twice for the following directories:

 - include/generated/
 - arch/*/include/generated/

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/package/install-extmod-build | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

Nicolas Schier July 31, 2024, 9:01 p.m. UTC | #1
On Sat, Jul 27, 2024 at 04:42:03PM +0900, Masahiro Yamada wrote:
> Exclude directories and files unnecessary for building external modules:
> 
>  - include/config/  (except include/config/auto.conf)
>  - scripts/atomic/
>  - scripts/dtc/
>  - scripts/kconfig/
>  - scripts/mod/mk_elfconfig
>  - scripts/package/
>  - scripts/unifdef
>  - .config
>  - *.o
>  - .*.cmd
> 
> Avoid copying files twice for the following directories:
> 
>  - include/generated/
>  - arch/*/include/generated/
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---

nice.

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>
Thomas Weißschuh Aug. 24, 2024, 12:27 p.m. UTC | #2
Hi Masahiro,

On 2024-07-27 16:42:03+0000, Masahiro Yamada wrote:
> Exclude directories and files unnecessary for building external modules:
> 
>  - include/config/  (except include/config/auto.conf)
>  - scripts/atomic/
>  - scripts/dtc/
>  - scripts/kconfig/
>  - scripts/mod/mk_elfconfig
>  - scripts/package/
>  - scripts/unifdef
>  - .config
>  - *.o
>  - .*.cmd
> 
> Avoid copying files twice for the following directories:
> 
>  - include/generated/
>  - arch/*/include/generated/
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  scripts/package/install-extmod-build | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/scripts/package/install-extmod-build b/scripts/package/install-extmod-build
> index 8cc9e13403ae..cc335945dfbc 100755
> --- a/scripts/package/install-extmod-build
> +++ b/scripts/package/install-extmod-build
> @@ -9,15 +9,22 @@ is_enabled() {
>  	grep -q "^$1=y" include/config/auto.conf
>  }
>  
> +find_in_scripts() {
> +	find scripts \
> +		\( -name atomic -o -name dtc -o -name kconfig -o -name package \) -prune -o \
> +		! -name unifdef -a ! -name mk_elfconfig -a \( -type f -o -type l \) -print
> +}
> +
>  mkdir -p "${destdir}"
>  
>  (
>  	cd "${srctree}"
>  	echo Makefile
>  	find "arch/${SRCARCH}" -maxdepth 1 -name 'Makefile*'
> -	find include scripts -type f -o -type l
> +	find "arch/${SRCARCH}" -name generated -prune -o -name include -type d -print
>  	find "arch/${SRCARCH}" -name Kbuild.platforms -o -name Platform
> -	find "arch/${SRCARCH}" -name include -type d
> +	find include \( -name config -o -name generated \) -prune -o \( -type f -o -type l \) -print
> +	find_in_scripts
>  ) | tar -c -f - -C "${srctree}" -T - | tar -xf - -C "${destdir}"
>  
>  {
> @@ -25,12 +32,15 @@ mkdir -p "${destdir}"
>  		echo tools/objtool/objtool
>  	fi
>  
> -	find "arch/${SRCARCH}/include" Module.symvers include scripts -type f
> +	echo Module.symvers
> +	echo "arch/${SRCARCH}/include/generated"
> +	echo include/config/auto.conf
> +	echo include/generated
> +	find_in_scripts

This now excludes include/config/kernel.release which is used to set
KERNELRELEASE, which is commonly used by Makefiles.
See Documentation/kbuild/modules.txt, other users also seem not unlikely.

IMO this specific file should be added back.


Thanks,
Thomas
Masahiro Yamada Aug. 24, 2024, 4:58 p.m. UTC | #3
On Sat, Aug 24, 2024 at 9:27 PM Thomas Weißschuh <thomas@t-8ch.de> wrote:
>
> Hi Masahiro,
>
> On 2024-07-27 16:42:03+0000, Masahiro Yamada wrote:
> > Exclude directories and files unnecessary for building external modules:
> >
> >  - include/config/  (except include/config/auto.conf)
> >  - scripts/atomic/
> >  - scripts/dtc/
> >  - scripts/kconfig/
> >  - scripts/mod/mk_elfconfig
> >  - scripts/package/
> >  - scripts/unifdef
> >  - .config
> >  - *.o
> >  - .*.cmd
> >
> > Avoid copying files twice for the following directories:
> >
> >  - include/generated/
> >  - arch/*/include/generated/
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/package/install-extmod-build | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/scripts/package/install-extmod-build b/scripts/package/install-extmod-build
> > index 8cc9e13403ae..cc335945dfbc 100755
> > --- a/scripts/package/install-extmod-build
> > +++ b/scripts/package/install-extmod-build
> > @@ -9,15 +9,22 @@ is_enabled() {
> >       grep -q "^$1=y" include/config/auto.conf
> >  }
> >
> > +find_in_scripts() {
> > +     find scripts \
> > +             \( -name atomic -o -name dtc -o -name kconfig -o -name package \) -prune -o \
> > +             ! -name unifdef -a ! -name mk_elfconfig -a \( -type f -o -type l \) -print
> > +}
> > +
> >  mkdir -p "${destdir}"
> >
> >  (
> >       cd "${srctree}"
> >       echo Makefile
> >       find "arch/${SRCARCH}" -maxdepth 1 -name 'Makefile*'
> > -     find include scripts -type f -o -type l
> > +     find "arch/${SRCARCH}" -name generated -prune -o -name include -type d -print
> >       find "arch/${SRCARCH}" -name Kbuild.platforms -o -name Platform
> > -     find "arch/${SRCARCH}" -name include -type d
> > +     find include \( -name config -o -name generated \) -prune -o \( -type f -o -type l \) -print
> > +     find_in_scripts
> >  ) | tar -c -f - -C "${srctree}" -T - | tar -xf - -C "${destdir}"
> >
> >  {
> > @@ -25,12 +32,15 @@ mkdir -p "${destdir}"
> >               echo tools/objtool/objtool
> >       fi
> >
> > -     find "arch/${SRCARCH}/include" Module.symvers include scripts -type f
> > +     echo Module.symvers
> > +     echo "arch/${SRCARCH}/include/generated"
> > +     echo include/config/auto.conf
> > +     echo include/generated
> > +     find_in_scripts
>
> This now excludes include/config/kernel.release which is used to set
> KERNELRELEASE, which is commonly used by Makefiles.
> See Documentation/kbuild/modules.txt, other users also seem not unlikely.
>
> IMO this specific file should be added back.


Agree.

I fixed it up locally. Thanks for the report.
Jeffrey Hugo Feb. 18, 2025, 8:25 p.m. UTC | #4
On 7/27/2024 1:42 AM, Masahiro Yamada wrote:
> Exclude directories and files unnecessary for building external modules:
> 
>   - include/config/  (except include/config/auto.conf)
>   - scripts/atomic/
>   - scripts/dtc/
>   - scripts/kconfig/
>   - scripts/mod/mk_elfconfig
>   - scripts/package/
>   - scripts/unifdef
>   - .config

Please revert this (the removal of .config).

I got some strange reports that our external module install broke, and 
traced it to this change.  Our external module references the .config 
because we have different logic for the build depending on if other, 
related modules are present or not.

Also, it looks like this broke DKMS for some configurations, which not 
only impacts DKMS itself [1] but also downstream projects [2].

While DKMS may be updated going forward to avoid this issue, there are 
plenty of affected version out in the wild.

Also, I haven't surveyed every distro, but it looks like Ubuntu still 
packages the .config with their headers in their upcoming "Plucky" 
release based on 6.12.  I suspect they wouldn't do that if they didn't 
feel it was needed/useful.

-Jeff

[1]: https://github.com/dell/dkms/issues/464
[2]: https://github.com/linux-surface/linux-surface/issues/1654
Nicolas Schier Feb. 20, 2025, 10:03 a.m. UTC | #5
On Tue, Feb 18, 2025 at 01:25:38PM -0700, Jeffrey Hugo wrote:
> On 7/27/2024 1:42 AM, Masahiro Yamada wrote:
> > Exclude directories and files unnecessary for building external modules:
> > 
> >   - include/config/  (except include/config/auto.conf)
> >   - scripts/atomic/
> >   - scripts/dtc/
> >   - scripts/kconfig/
> >   - scripts/mod/mk_elfconfig
> >   - scripts/package/
> >   - scripts/unifdef
> >   - .config
> 
> Please revert this (the removal of .config).
> 
> I got some strange reports that our external module install broke, and
> traced it to this change.  Our external module references the .config
> because we have different logic for the build depending on if other, related
> modules are present or not.
> 
> Also, it looks like this broke DKMS for some configurations, which not only
> impacts DKMS itself [1] but also downstream projects [2].
> 
> While DKMS may be updated going forward to avoid this issue, there are
> plenty of affected version out in the wild.
> 
> Also, I haven't surveyed every distro, but it looks like Ubuntu still
> packages the .config with their headers in their upcoming "Plucky" release
> based on 6.12.  I suspect they wouldn't do that if they didn't feel it was
> needed/useful.
> 
> -Jeff
> 
> [1]: https://github.com/dell/dkms/issues/464
> [2]: https://github.com/linux-surface/linux-surface/issues/1654

Hi Jeff,

do you know the related thread [1]?  According to the last mail, DKMS
has fixed its issue already in December.

Kind regards,
Nicolas

[1]: https://lore.kernel.org/linux-kbuild/CAK7LNARqEOVOzP5vdUVF0KxQBNb9xtYs-COSXXWDMpBzGaLGow@mail.gmail.com/T/#m95f48caf46357a41c1df5e038e227a01ab89dbda
Jeffrey Hugo Feb. 20, 2025, 3:03 p.m. UTC | #6
On 2/20/2025 3:03 AM, Nicolas Schier wrote:
> On Tue, Feb 18, 2025 at 01:25:38PM -0700, Jeffrey Hugo wrote:
>> On 7/27/2024 1:42 AM, Masahiro Yamada wrote:
>>> Exclude directories and files unnecessary for building external modules:
>>>
>>>    - include/config/  (except include/config/auto.conf)
>>>    - scripts/atomic/
>>>    - scripts/dtc/
>>>    - scripts/kconfig/
>>>    - scripts/mod/mk_elfconfig
>>>    - scripts/package/
>>>    - scripts/unifdef
>>>    - .config
>>
>> Please revert this (the removal of .config).
>>
>> I got some strange reports that our external module install broke, and
>> traced it to this change.  Our external module references the .config
>> because we have different logic for the build depending on if other, related
>> modules are present or not.
>>
>> Also, it looks like this broke DKMS for some configurations, which not only
>> impacts DKMS itself [1] but also downstream projects [2].
>>
>> While DKMS may be updated going forward to avoid this issue, there are
>> plenty of affected version out in the wild.
>>
>> Also, I haven't surveyed every distro, but it looks like Ubuntu still
>> packages the .config with their headers in their upcoming "Plucky" release
>> based on 6.12.  I suspect they wouldn't do that if they didn't feel it was
>> needed/useful.
>>
>> -Jeff
>>
>> [1]: https://github.com/dell/dkms/issues/464
>> [2]: https://github.com/linux-surface/linux-surface/issues/1654
> 
> Hi Jeff,
> 
> do you know the related thread [1]?  According to the last mail, DKMS
> has fixed its issue already in December.

DKMS tip might be fixed, but production versions are in various distros, 
which are not updated.  Therefore actual users are impacted by this.

What happened to the #1 rule of kernel, that we do not break users?

This still needs to be reverted.

-Jeff

> 
> Kind regards,
> Nicolas
> 
> [1]: https://lore.kernel.org/linux-kbuild/CAK7LNARqEOVOzP5vdUVF0KxQBNb9xtYs-COSXXWDMpBzGaLGow@mail.gmail.com/T/#m95f48caf46357a41c1df5e038e227a01ab89dbda
Nicolas Schier Feb. 20, 2025, 3:54 p.m. UTC | #7
On Thu, Feb 20, 2025 at 08:03:32AM -0700, Jeffrey Hugo wrote:
> On 2/20/2025 3:03 AM, Nicolas Schier wrote:
> > On Tue, Feb 18, 2025 at 01:25:38PM -0700, Jeffrey Hugo wrote:
> > > On 7/27/2024 1:42 AM, Masahiro Yamada wrote:
> > > > Exclude directories and files unnecessary for building external modules:
> > > > 
> > > >    - include/config/  (except include/config/auto.conf)
> > > >    - scripts/atomic/
> > > >    - scripts/dtc/
> > > >    - scripts/kconfig/
> > > >    - scripts/mod/mk_elfconfig
> > > >    - scripts/package/
> > > >    - scripts/unifdef
> > > >    - .config
> > > 
> > > Please revert this (the removal of .config).
> > > 
> > > I got some strange reports that our external module install broke, and
> > > traced it to this change.  Our external module references the .config
> > > because we have different logic for the build depending on if other, related
> > > modules are present or not.
> > > 
> > > Also, it looks like this broke DKMS for some configurations, which not only
> > > impacts DKMS itself [1] but also downstream projects [2].
> > > 
> > > While DKMS may be updated going forward to avoid this issue, there are
> > > plenty of affected version out in the wild.
> > > 
> > > Also, I haven't surveyed every distro, but it looks like Ubuntu still
> > > packages the .config with their headers in their upcoming "Plucky" release
> > > based on 6.12.  I suspect they wouldn't do that if they didn't feel it was
> > > needed/useful.
> > > 
> > > -Jeff
> > > 
> > > [1]: https://github.com/dell/dkms/issues/464
> > > [2]: https://github.com/linux-surface/linux-surface/issues/1654
> > 
> > Hi Jeff,
> > 
> > do you know the related thread [1]?  According to the last mail, DKMS
> > has fixed its issue already in December.
> 
> DKMS tip might be fixed, but production versions are in various distros,
> which are not updated.  Therefore actual users are impacted by this.
> 
> What happened to the #1 rule of kernel, that we do not break users?

I think, Masahiro already provided valid and profound reasons for
keeping it the way it is.  Users of run-time kernel interfaces are not
affected by the change.  Concretely reported issues were, as far as I
know, only a matter of specific non-official way to work with .config
for other reasons than just building external kernel modules in the way
it is thought to work.

Kind regards,
Nicolas

> This still needs to be reverted.
> 
> -Jeff
> 
> > 
> > Kind regards,
> > Nicolas
> > 
> > [1]: https://lore.kernel.org/linux-kbuild/CAK7LNARqEOVOzP5vdUVF0KxQBNb9xtYs-COSXXWDMpBzGaLGow@mail.gmail.com/T/#m95f48caf46357a41c1df5e038e227a01ab89dbda
>
Jeffrey Hugo Feb. 20, 2025, 4:31 p.m. UTC | #8
On 2/20/2025 8:54 AM, Nicolas Schier wrote:
> On Thu, Feb 20, 2025 at 08:03:32AM -0700, Jeffrey Hugo wrote:
>> On 2/20/2025 3:03 AM, Nicolas Schier wrote:
>>> On Tue, Feb 18, 2025 at 01:25:38PM -0700, Jeffrey Hugo wrote:
>>>> On 7/27/2024 1:42 AM, Masahiro Yamada wrote:
>>>>> Exclude directories and files unnecessary for building external modules:
>>>>>
>>>>>     - include/config/  (except include/config/auto.conf)
>>>>>     - scripts/atomic/
>>>>>     - scripts/dtc/
>>>>>     - scripts/kconfig/
>>>>>     - scripts/mod/mk_elfconfig
>>>>>     - scripts/package/
>>>>>     - scripts/unifdef
>>>>>     - .config
>>>>
>>>> Please revert this (the removal of .config).
>>>>
>>>> I got some strange reports that our external module install broke, and
>>>> traced it to this change.  Our external module references the .config
>>>> because we have different logic for the build depending on if other, related
>>>> modules are present or not.
>>>>
>>>> Also, it looks like this broke DKMS for some configurations, which not only
>>>> impacts DKMS itself [1] but also downstream projects [2].
>>>>
>>>> While DKMS may be updated going forward to avoid this issue, there are
>>>> plenty of affected version out in the wild.
>>>>
>>>> Also, I haven't surveyed every distro, but it looks like Ubuntu still
>>>> packages the .config with their headers in their upcoming "Plucky" release
>>>> based on 6.12.  I suspect they wouldn't do that if they didn't feel it was
>>>> needed/useful.
>>>>
>>>> -Jeff
>>>>
>>>> [1]: https://github.com/dell/dkms/issues/464
>>>> [2]: https://github.com/linux-surface/linux-surface/issues/1654
>>>
>>> Hi Jeff,
>>>
>>> do you know the related thread [1]?  According to the last mail, DKMS
>>> has fixed its issue already in December.
>>
>> DKMS tip might be fixed, but production versions are in various distros,
>> which are not updated.  Therefore actual users are impacted by this.
>>
>> What happened to the #1 rule of kernel, that we do not break users?
> 
> I think, Masahiro already provided valid and profound reasons for
> keeping it the way it is.  Users of run-time kernel interfaces are not
> affected by the change.  Concretely reported issues were, as far as I
> know, only a matter of specific non-official way to work with .config
> for other reasons than just building external kernel modules in the way
> it is thought to work.

I'm CCing the regressions list, which I probably should have done initially.

I'm pretty sure Linus has indicated that as soon as users start doing 
something, that becomes the "official way".  I believe your response is 
not consistent with the precedent set by Linus.

Quoting from [1]:
It’s a regression if some application or practical use case running fine 
with one Linux kernel works worse or not at all with a newer version 
compiled using a similar configuration. The “no regressions” rule 
forbids this to take place; if it happens by accident, developers that 
caused it are expected to quickly fix the issue."

As far as I understand its not acceptable to force users to change (the 
DKMS fix) or update userspace to work with a new kernel.  You could make 
a change if userspace updated, and all old versions needing the previous 
behavior were phased out of use, but we have 2 reports indicating that 
is not the case.

 From the thread you pointed out, it looks like Masahiro recommends 
${kernel_source_dir}/include/config/auto.conf as a replacement for the 
.config.  Ignoring that such a suggestion seems to violate the 
regressions rule, doing a diff of that and the .config on a 6.11 build 
(before the change we are discussing), there are many changes.  It does 
not look like that is an equivalent replacement.

Are we at an impasse?  Masahiro has not chimed in, which is quite 
disappointing.  So far, I don't think your responses justify why 
breaking users is acceptable.  This is not a security fix - the only 
justification for this change is that .config is not needed, but I argue 
that has been proven false.  It seems like I am not convincing you.

-Jeff

[1]: https://docs.kernel.org/admin-guide/reporting-regressions.html

> Kind regards,
> Nicolas
> 
>> This still needs to be reverted.
>>
>> -Jeff
>>
>>>
>>> Kind regards,
>>> Nicolas
>>>
>>> [1]: https://lore.kernel.org/linux-kbuild/CAK7LNARqEOVOzP5vdUVF0KxQBNb9xtYs-COSXXWDMpBzGaLGow@mail.gmail.com/T/#m95f48caf46357a41c1df5e038e227a01ab89dbda
>>
Greg KH Feb. 20, 2025, 4:49 p.m. UTC | #9
On Thu, Feb 20, 2025 at 09:31:14AM -0700, Jeffrey Hugo wrote:
> On 2/20/2025 8:54 AM, Nicolas Schier wrote:
> > On Thu, Feb 20, 2025 at 08:03:32AM -0700, Jeffrey Hugo wrote:
> > > On 2/20/2025 3:03 AM, Nicolas Schier wrote:
> > > > On Tue, Feb 18, 2025 at 01:25:38PM -0700, Jeffrey Hugo wrote:
> > > > > On 7/27/2024 1:42 AM, Masahiro Yamada wrote:
> > > > > > Exclude directories and files unnecessary for building external modules:
> > > > > > 
> > > > > >     - include/config/  (except include/config/auto.conf)
> > > > > >     - scripts/atomic/
> > > > > >     - scripts/dtc/
> > > > > >     - scripts/kconfig/
> > > > > >     - scripts/mod/mk_elfconfig
> > > > > >     - scripts/package/
> > > > > >     - scripts/unifdef
> > > > > >     - .config
> > > > > 
> > > > > Please revert this (the removal of .config).
> > > > > 
> > > > > I got some strange reports that our external module install broke, and
> > > > > traced it to this change.  Our external module references the .config
> > > > > because we have different logic for the build depending on if other, related
> > > > > modules are present or not.
> > > > > 
> > > > > Also, it looks like this broke DKMS for some configurations, which not only
> > > > > impacts DKMS itself [1] but also downstream projects [2].
> > > > > 
> > > > > While DKMS may be updated going forward to avoid this issue, there are
> > > > > plenty of affected version out in the wild.
> > > > > 
> > > > > Also, I haven't surveyed every distro, but it looks like Ubuntu still
> > > > > packages the .config with their headers in their upcoming "Plucky" release
> > > > > based on 6.12.  I suspect they wouldn't do that if they didn't feel it was
> > > > > needed/useful.
> > > > > 
> > > > > -Jeff
> > > > > 
> > > > > [1]: https://github.com/dell/dkms/issues/464
> > > > > [2]: https://github.com/linux-surface/linux-surface/issues/1654
> > > > 
> > > > Hi Jeff,
> > > > 
> > > > do you know the related thread [1]?  According to the last mail, DKMS
> > > > has fixed its issue already in December.
> > > 
> > > DKMS tip might be fixed, but production versions are in various distros,
> > > which are not updated.  Therefore actual users are impacted by this.
> > > 
> > > What happened to the #1 rule of kernel, that we do not break users?
> > 
> > I think, Masahiro already provided valid and profound reasons for
> > keeping it the way it is.  Users of run-time kernel interfaces are not
> > affected by the change.  Concretely reported issues were, as far as I
> > know, only a matter of specific non-official way to work with .config
> > for other reasons than just building external kernel modules in the way
> > it is thought to work.
> 
> I'm CCing the regressions list, which I probably should have done initially.
> 
> I'm pretty sure Linus has indicated that as soon as users start doing
> something, that becomes the "official way".  I believe your response is not
> consistent with the precedent set by Linus.
> 
> Quoting from [1]:
> It’s a regression if some application or practical use case running fine
> with one Linux kernel works worse or not at all with a newer version
> compiled using a similar configuration. The “no regressions” rule forbids
> this to take place; if it happens by accident, developers that caused it are
> expected to quickly fix the issue."

Regressions are at runtime, not with how external tools poke around in
kernel source trees and try to make decisions.  If we were to never be
able to change our source just because there might be an external user
that we don't know of, that would be crazy.

We want users to not be afraid to upgrade their kernel, that has nothing
to do with how the kernel build is interacted with external stuff.

> As far as I understand its not acceptable to force users to change (the DKMS
> fix) or update userspace to work with a new kernel.  You could make a change
> if userspace updated, and all old versions needing the previous behavior
> were phased out of use, but we have 2 reports indicating that is not the
> case.

You are attempting to build kernel modules outside of the kernel tree,
and as such, have to adapt to things when they change.  That's always
been the case, you've had to change things many times over the years,
right?

> From the thread you pointed out, it looks like Masahiro recommends
> ${kernel_source_dir}/include/config/auto.conf as a replacement for the
> .config.  Ignoring that such a suggestion seems to violate the regressions
> rule, doing a diff of that and the .config on a 6.11 build (before the
> change we are discussing), there are many changes.  It does not look like
> that is an equivalent replacement.

What do you need/want to parse the .config file in the first place?  Why
not rely on the generated .h files instead as those can be parsed the
same way as before, right?

thanks,

greg k-h
Jeffrey Hugo Feb. 20, 2025, 5:24 p.m. UTC | #10
On 2/20/2025 9:49 AM, Greg KH wrote:
> On Thu, Feb 20, 2025 at 09:31:14AM -0700, Jeffrey Hugo wrote:
>> On 2/20/2025 8:54 AM, Nicolas Schier wrote:
>>> On Thu, Feb 20, 2025 at 08:03:32AM -0700, Jeffrey Hugo wrote:
>>>> On 2/20/2025 3:03 AM, Nicolas Schier wrote:
>>>>> On Tue, Feb 18, 2025 at 01:25:38PM -0700, Jeffrey Hugo wrote:
>>>>>> On 7/27/2024 1:42 AM, Masahiro Yamada wrote:
>>>>>>> Exclude directories and files unnecessary for building external modules:
>>>>>>>
>>>>>>>      - include/config/  (except include/config/auto.conf)
>>>>>>>      - scripts/atomic/
>>>>>>>      - scripts/dtc/
>>>>>>>      - scripts/kconfig/
>>>>>>>      - scripts/mod/mk_elfconfig
>>>>>>>      - scripts/package/
>>>>>>>      - scripts/unifdef
>>>>>>>      - .config
>>>>>>
>>>>>> Please revert this (the removal of .config).
>>>>>>
>>>>>> I got some strange reports that our external module install broke, and
>>>>>> traced it to this change.  Our external module references the .config
>>>>>> because we have different logic for the build depending on if other, related
>>>>>> modules are present or not.
>>>>>>
>>>>>> Also, it looks like this broke DKMS for some configurations, which not only
>>>>>> impacts DKMS itself [1] but also downstream projects [2].
>>>>>>
>>>>>> While DKMS may be updated going forward to avoid this issue, there are
>>>>>> plenty of affected version out in the wild.
>>>>>>
>>>>>> Also, I haven't surveyed every distro, but it looks like Ubuntu still
>>>>>> packages the .config with their headers in their upcoming "Plucky" release
>>>>>> based on 6.12.  I suspect they wouldn't do that if they didn't feel it was
>>>>>> needed/useful.
>>>>>>
>>>>>> -Jeff
>>>>>>
>>>>>> [1]: https://github.com/dell/dkms/issues/464
>>>>>> [2]: https://github.com/linux-surface/linux-surface/issues/1654
>>>>>
>>>>> Hi Jeff,
>>>>>
>>>>> do you know the related thread [1]?  According to the last mail, DKMS
>>>>> has fixed its issue already in December.
>>>>
>>>> DKMS tip might be fixed, but production versions are in various distros,
>>>> which are not updated.  Therefore actual users are impacted by this.
>>>>
>>>> What happened to the #1 rule of kernel, that we do not break users?
>>>
>>> I think, Masahiro already provided valid and profound reasons for
>>> keeping it the way it is.  Users of run-time kernel interfaces are not
>>> affected by the change.  Concretely reported issues were, as far as I
>>> know, only a matter of specific non-official way to work with .config
>>> for other reasons than just building external kernel modules in the way
>>> it is thought to work.
>>
>> I'm CCing the regressions list, which I probably should have done initially.
>>
>> I'm pretty sure Linus has indicated that as soon as users start doing
>> something, that becomes the "official way".  I believe your response is not
>> consistent with the precedent set by Linus.
>>
>> Quoting from [1]:
>> It’s a regression if some application or practical use case running fine
>> with one Linux kernel works worse or not at all with a newer version
>> compiled using a similar configuration. The “no regressions” rule forbids
>> this to take place; if it happens by accident, developers that caused it are
>> expected to quickly fix the issue."
> 
> Regressions are at runtime, not with how external tools poke around in
> kernel source trees and try to make decisions.  If we were to never be
> able to change our source just because there might be an external user
> that we don't know of, that would be crazy.

As a kernel developer, I get this.  The MAX_ORDER thing from a few 
versions ago seemed to make this a grey area.  Perhaps it a case by case 
thing.  In which case, what is the harm in sharing the kernel's .config 
which has been a thing since I started kernel development back in the 
2.6.X times?

> We want users to not be afraid to upgrade their kernel, that has nothing
> to do with how the kernel build is interacted with external stuff.
 > >> As far as I understand its not acceptable to force users to change 
(the DKMS
>> fix) or update userspace to work with a new kernel.  You could make a change
>> if userspace updated, and all old versions needing the previous behavior
>> were phased out of use, but we have 2 reports indicating that is not the
>> case.
> 
> You are attempting to build kernel modules outside of the kernel tree,
> and as such, have to adapt to things when they change.  That's always
> been the case, you've had to change things many times over the years,
> right?

While true, its possible to build an external module against 6.11 and 
6.12 and still hit a failure because of this change (see below about DKMS).

> 
>>  From the thread you pointed out, it looks like Masahiro recommends
>> ${kernel_source_dir}/include/config/auto.conf as a replacement for the
>> .config.  Ignoring that such a suggestion seems to violate the regressions
>> rule, doing a diff of that and the .config on a 6.11 build (before the
>> change we are discussing), there are many changes.  It does not look like
>> that is an equivalent replacement.
> 
> What do you need/want to parse the .config file in the first place?  Why
> not rely on the generated .h files instead as those can be parsed the
> same way as before, right?

Two usecases -

First when secure boot is enabled, DKMS looks for CONFIG_MODULE_SIG_HASH 
to figure out signing the modules so that they can be loaded.  Removing 
.config causes an error in DKMS and the module signing process will 
fail.  The resulting modules (already compiled successfully) will fail 
to load.  Whatever the user needed those modules for will no longer work.

If the user is on Ubuntu, and has built a kernel 6.12 or later, they 
need to build upstream DKMS and use it as none of the Canonical provided 
DKMS builds have the fix.  This feels like a situation that would make 
the user afraid to upgrade their kernel (to your point above).

This feels very much like an "at runtime" issue, assuming external 
modules are supported.  I may be wrong here.

I'm not a DKMS developer, but I do use it, and find it extremely handy 
to take latest upstream code and apply it older kernels that customers 
seem to want to use.

Second, our usecase is that we look at the .config to tell if a 
particular driver is included in the kernel build (config =y or =m). 
This driver provides diagnostic information which is useful to our 
product, but not required for operation.  It does not have headers that 
are exposed to the rest of the kernel, so checking the generated .h 
files does not work.  If the driver is not built, we provide a 
backported version that is then built out of tree.

I can, with effort, extend the logic to first look for a .config (since 
the distros provide that), and if not, then look for the auto.conf 
(assuming that doesn't get determined to be not useful) to kick off our 
logic.  We'd still face the DKMS issue above so my preference would be a 
revert since it addresses both problems.

-Jeff
Greg KH Feb. 20, 2025, 5:43 p.m. UTC | #11
On Thu, Feb 20, 2025 at 10:24:32AM -0700, Jeffrey Hugo wrote:
> On 2/20/2025 9:49 AM, Greg KH wrote:
> > What do you need/want to parse the .config file in the first place?  Why
> > not rely on the generated .h files instead as those can be parsed the
> > same way as before, right?
> 
> Two usecases -
> 
> First when secure boot is enabled, DKMS looks for CONFIG_MODULE_SIG_HASH to
> figure out signing the modules so that they can be loaded.  Removing .config
> causes an error in DKMS and the module signing process will fail.  The
> resulting modules (already compiled successfully) will fail to load.
> Whatever the user needed those modules for will no longer work.

Shouldn't the "normal" kbuild process properly sign the module if it
needs to be signed?  Or are you trying to do this "by hand"?

> If the user is on Ubuntu, and has built a kernel 6.12 or later, they need to
> build upstream DKMS and use it as none of the Canonical provided DKMS builds
> have the fix.  This feels like a situation that would make the user afraid
> to upgrade their kernel (to your point above).
> 
> This feels very much like an "at runtime" issue, assuming external modules
> are supported.  I may be wrong here.

external modules can be broken at any moment in time, you know that.
There's never a guarantee for that at all.

> Second, our usecase is that we look at the .config to tell if a particular
> driver is included in the kernel build (config =y or =m). This driver
> provides diagnostic information which is useful to our product, but not
> required for operation.  It does not have headers that are exposed to the
> rest of the kernel, so checking the generated .h files does not work.  If
> the driver is not built, we provide a backported version that is then built
> out of tree.

You can check the same .h files for those config options, no need to
manually parse a .config file.  What's wrong with including
include/generated/autoconf.h properly?  That's what the build system
uses, right?

thanks,

greg k-h
Masahiro Yamada Feb. 20, 2025, 5:57 p.m. UTC | #12
On Fri, Feb 21, 2025 at 2:24 AM Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
>
> On 2/20/2025 9:49 AM, Greg KH wrote:
> > On Thu, Feb 20, 2025 at 09:31:14AM -0700, Jeffrey Hugo wrote:
> >> On 2/20/2025 8:54 AM, Nicolas Schier wrote:
> >>> On Thu, Feb 20, 2025 at 08:03:32AM -0700, Jeffrey Hugo wrote:
> >>>> On 2/20/2025 3:03 AM, Nicolas Schier wrote:
> >>>>> On Tue, Feb 18, 2025 at 01:25:38PM -0700, Jeffrey Hugo wrote:
> >>>>>> On 7/27/2024 1:42 AM, Masahiro Yamada wrote:
> >>>>>>> Exclude directories and files unnecessary for building external modules:
> >>>>>>>
> >>>>>>>      - include/config/  (except include/config/auto.conf)
> >>>>>>>      - scripts/atomic/
> >>>>>>>      - scripts/dtc/
> >>>>>>>      - scripts/kconfig/
> >>>>>>>      - scripts/mod/mk_elfconfig
> >>>>>>>      - scripts/package/
> >>>>>>>      - scripts/unifdef
> >>>>>>>      - .config
> >>>>>>
> >>>>>> Please revert this (the removal of .config).
> >>>>>>
> >>>>>> I got some strange reports that our external module install broke, and
> >>>>>> traced it to this change.  Our external module references the .config
> >>>>>> because we have different logic for the build depending on if other, related
> >>>>>> modules are present or not.
> >>>>>>
> >>>>>> Also, it looks like this broke DKMS for some configurations, which not only
> >>>>>> impacts DKMS itself [1] but also downstream projects [2].
> >>>>>>
> >>>>>> While DKMS may be updated going forward to avoid this issue, there are
> >>>>>> plenty of affected version out in the wild.
> >>>>>>
> >>>>>> Also, I haven't surveyed every distro, but it looks like Ubuntu still
> >>>>>> packages the .config with their headers in their upcoming "Plucky" release
> >>>>>> based on 6.12.  I suspect they wouldn't do that if they didn't feel it was
> >>>>>> needed/useful.
> >>>>>>
> >>>>>> -Jeff
> >>>>>>
> >>>>>> [1]: https://github.com/dell/dkms/issues/464
> >>>>>> [2]: https://github.com/linux-surface/linux-surface/issues/1654
> >>>>>
> >>>>> Hi Jeff,
> >>>>>
> >>>>> do you know the related thread [1]?  According to the last mail, DKMS
> >>>>> has fixed its issue already in December.
> >>>>
> >>>> DKMS tip might be fixed, but production versions are in various distros,
> >>>> which are not updated.  Therefore actual users are impacted by this.
> >>>>
> >>>> What happened to the #1 rule of kernel, that we do not break users?
> >>>
> >>> I think, Masahiro already provided valid and profound reasons for
> >>> keeping it the way it is.  Users of run-time kernel interfaces are not
> >>> affected by the change.  Concretely reported issues were, as far as I
> >>> know, only a matter of specific non-official way to work with .config
> >>> for other reasons than just building external kernel modules in the way
> >>> it is thought to work.
> >>
> >> I'm CCing the regressions list, which I probably should have done initially.
> >>
> >> I'm pretty sure Linus has indicated that as soon as users start doing
> >> something, that becomes the "official way".  I believe your response is not
> >> consistent with the precedent set by Linus.
> >>
> >> Quoting from [1]:
> >> It’s a regression if some application or practical use case running fine
> >> with one Linux kernel works worse or not at all with a newer version
> >> compiled using a similar configuration. The “no regressions” rule forbids
> >> this to take place; if it happens by accident, developers that caused it are
> >> expected to quickly fix the issue."
> >
> > Regressions are at runtime, not with how external tools poke around in
> > kernel source trees and try to make decisions.  If we were to never be
> > able to change our source just because there might be an external user
> > that we don't know of, that would be crazy.
>
> As a kernel developer, I get this.  The MAX_ORDER thing from a few
> versions ago seemed to make this a grey area.  Perhaps it a case by case
> thing.  In which case, what is the harm in sharing the kernel's .config
> which has been a thing since I started kernel development back in the
> 2.6.X times?
>
> > We want users to not be afraid to upgrade their kernel, that has nothing
> > to do with how the kernel build is interacted with external stuff.
>  > >> As far as I understand its not acceptable to force users to change
> (the DKMS
> >> fix) or update userspace to work with a new kernel.  You could make a change
> >> if userspace updated, and all old versions needing the previous behavior
> >> were phased out of use, but we have 2 reports indicating that is not the
> >> case.
> >
> > You are attempting to build kernel modules outside of the kernel tree,
> > and as such, have to adapt to things when they change.  That's always
> > been the case, you've had to change things many times over the years,
> > right?
>
> While true, its possible to build an external module against 6.11 and
> 6.12 and still hit a failure because of this change (see below about DKMS).
>
> >
> >>  From the thread you pointed out, it looks like Masahiro recommends
> >> ${kernel_source_dir}/include/config/auto.conf as a replacement for the
> >> .config.  Ignoring that such a suggestion seems to violate the regressions
> >> rule, doing a diff of that and the .config on a 6.11 build (before the
> >> change we are discussing), there are many changes.  It does not look like
> >> that is an equivalent replacement.
> >
> > What do you need/want to parse the .config file in the first place?  Why
> > not rely on the generated .h files instead as those can be parsed the
> > same way as before, right?
>
> Two usecases -
>
> First when secure boot is enabled, DKMS looks for CONFIG_MODULE_SIG_HASH
> to figure out signing the modules so that they can be loaded.  Removing
> .config causes an error in DKMS and the module signing process will
> fail.  The resulting modules (already compiled successfully) will fail
> to load.  Whatever the user needed those modules for will no longer work.
>
> If the user is on Ubuntu, and has built a kernel 6.12 or later, they
> need to build upstream DKMS and use it as none of the Canonical provided
> DKMS builds have the fix.  This feels like a situation that would make
> the user afraid to upgrade their kernel (to your point above).
>
> This feels very much like an "at runtime" issue, assuming external
> modules are supported.  I may be wrong here.
>
> I'm not a DKMS developer, but I do use it, and find it extremely handy
> to take latest upstream code and apply it older kernels that customers
> seem to want to use.
>
> Second, our usecase is that we look at the .config to tell if a
> particular driver is included in the kernel build (config =y or =m).
> This driver provides diagnostic information which is useful to our
> product, but not required for operation.  It does not have headers that
> are exposed to the rest of the kernel, so checking the generated .h
> files does not work.  If the driver is not built, we provide a
> backported version that is then built out of tree.

External modules should not parse the .config.
It was documented 20 years ago.

See the bottom of this:
https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=65e433436b5794ae056d22ddba60fe9194bba007

"External modules have traditionally used grep to check for specific
 CONFIG_ settings directly in .config. This usage is broken.
As introduced before external modules shall use kbuild when building
and therefore can use the same methods as in-kernel modules when testing
for CONFIG_ definitions.
"

The commit date is 2004.

The .config is a saved kernel configuration.
Kbuild does not use it at compile time.
External modules should follow the Kbuild rule
(that is, use include/config/auto.conf instead)


While commit 1a59bd3ca5d8fde10d082e56c3073f7fa563e73b
deleted it, the reason is this information is stale.
If we do not ship the .config in the package,
we do not need to say "do not parse .config".


You repeatedly fail to understand the regression in this context.
We promise backward-compatibility for userspace interfaces.
(system calls, UAPI headers, etc.)

We do not promise anything to external modules:
 - Header files under include/ (except uapi/) are changed without caring
   about the breakage of external modules
 - EXPORT_SYMBOL is dropped if no user is present in upstream



>
> I can, with effort, extend the logic to first look for a .config (since
> the distros provide that), and if not, then look for the auto.conf
> (assuming that doesn't get determined to be not useful) to kick off our
> logic.  We'd still face the DKMS issue above so my preference would be a
> revert since it addresses both problems.
>
> -Jeff
>
>
>
Masahiro Yamada Feb. 20, 2025, 6:12 p.m. UTC | #13
On Fri, Feb 21, 2025 at 2:43 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Feb 20, 2025 at 10:24:32AM -0700, Jeffrey Hugo wrote:
> > On 2/20/2025 9:49 AM, Greg KH wrote:
> > > What do you need/want to parse the .config file in the first place?  Why
> > > not rely on the generated .h files instead as those can be parsed the
> > > same way as before, right?
> >
> > Two usecases -
> >
> > First when secure boot is enabled, DKMS looks for CONFIG_MODULE_SIG_HASH to
> > figure out signing the modules so that they can be loaded.  Removing .config
> > causes an error in DKMS and the module signing process will fail.  The
> > resulting modules (already compiled successfully) will fail to load.
> > Whatever the user needed those modules for will no longer work.
>
> Shouldn't the "normal" kbuild process properly sign the module if it
> needs to be signed?  Or are you trying to do this "by hand"?

You can ignore this.
This is not related to the upstream module signing.

He is just explaining how DKMS greps CONFIG_MODULE_SIG_HASH.

See this line:
https://github.com/dell/dkms/blob/v3.1.5/dkms.in#L1113

The upstream kernel documentation said "do not grep .config" many years ago.
The latest DKMS has been fixed.



>
> > If the user is on Ubuntu, and has built a kernel 6.12 or later, they need to
> > build upstream DKMS and use it as none of the Canonical provided DKMS builds
> > have the fix.  This feels like a situation that would make the user afraid
> > to upgrade their kernel (to your point above).
> >
> > This feels very much like an "at runtime" issue, assuming external modules
> > are supported.  I may be wrong here.
>
> external modules can be broken at any moment in time, you know that.
> There's never a guarantee for that at all.
>
> > Second, our usecase is that we look at the .config to tell if a particular
> > driver is included in the kernel build (config =y or =m). This driver
> > provides diagnostic information which is useful to our product, but not
> > required for operation.  It does not have headers that are exposed to the
> > rest of the kernel, so checking the generated .h files does not work.  If
> > the driver is not built, we provide a backported version that is then built
> > out of tree.
>
> You can check the same .h files for those config options, no need to
> manually parse a .config file.  What's wrong with including
> include/generated/autoconf.h properly?  That's what the build system
> uses, right?

Upstream uses include/config/auto.conf
External modules can do the same.
diff mbox series

Patch

diff --git a/scripts/package/install-extmod-build b/scripts/package/install-extmod-build
index 8cc9e13403ae..cc335945dfbc 100755
--- a/scripts/package/install-extmod-build
+++ b/scripts/package/install-extmod-build
@@ -9,15 +9,22 @@  is_enabled() {
 	grep -q "^$1=y" include/config/auto.conf
 }
 
+find_in_scripts() {
+	find scripts \
+		\( -name atomic -o -name dtc -o -name kconfig -o -name package \) -prune -o \
+		! -name unifdef -a ! -name mk_elfconfig -a \( -type f -o -type l \) -print
+}
+
 mkdir -p "${destdir}"
 
 (
 	cd "${srctree}"
 	echo Makefile
 	find "arch/${SRCARCH}" -maxdepth 1 -name 'Makefile*'
-	find include scripts -type f -o -type l
+	find "arch/${SRCARCH}" -name generated -prune -o -name include -type d -print
 	find "arch/${SRCARCH}" -name Kbuild.platforms -o -name Platform
-	find "arch/${SRCARCH}" -name include -type d
+	find include \( -name config -o -name generated \) -prune -o \( -type f -o -type l \) -print
+	find_in_scripts
 ) | tar -c -f - -C "${srctree}" -T - | tar -xf - -C "${destdir}"
 
 {
@@ -25,12 +32,15 @@  mkdir -p "${destdir}"
 		echo tools/objtool/objtool
 	fi
 
-	find "arch/${SRCARCH}/include" Module.symvers include scripts -type f
+	echo Module.symvers
+	echo "arch/${SRCARCH}/include/generated"
+	echo include/config/auto.conf
+	echo include/generated
+	find_in_scripts
 
 	if is_enabled CONFIG_GCC_PLUGINS; then
 		find scripts/gcc-plugins -name '*.so'
 	fi
 } | tar -c -f - -T - | tar -xf - -C "${destdir}"
 
-# copy .config manually to be where it's expected to be
-cp "${KCONFIG_CONFIG}" "${destdir}/.config"
+find "${destdir}" \( -name '.*.cmd' -o -name '*.o' \) -delete