Re: [PATCH] build some drivers only when compile-testing

Hi,

On Tue, Jun 18, 2013 at 06:51:32AM +0200, Michal Marek wrote:

Dne 17.6.2013 22:05, Jiri Slaby napsal(a):

quoted

On 05/23/2013 05:09 AM, Jeff Mahoney wrote:

quoted

On 5/22/13 10:23 PM, Greg Kroah-Hartman wrote:

quoted

On Wed, May 22, 2013 at 11:18:46AM +0200, Jiri Slaby wrote:

quoted

Some drivers can be built on more platforms than they run on. This
causes users and distributors packaging burden when they have to
manually deselect some drivers from their allmodconfigs. Or sometimes
it is even impossible to disable the drivers without patching the
kernel.

Introduce a new config option COMPILE_TEST and make all those drivers
to depend on the platform they run on, or on the COMPILE_TEST option.
Now, when users/distributors choose COMPILE_TEST=n they will not have
the drivers in their allmodconfig setups, but developers still can
compile-test them with COMPILE_TEST=y.

I understand the urge, and it's getting hard for distros to handle these
drivers that just don't work on other architectures, but it's really
valuable to ensure that they build properly, for those of us that don't
have many/any cross compilers set up.

But this is exactly what COMPILE_TEST will give us when set to "y", or
am I missing something?

quoted

quoted

quoted

Now the drivers where we use this new option:
* PTP_1588_CLOCK_PCH: The PCH EG20T is only compatible with Intel Atom
  processors so it should depend on x86.
* FB_GEODE: Geode is 32-bit only so only enable it for X86_32.
* USB_CHIPIDEA_IMX: The OF_DEVICE dependency will be met on powerpc
  systems -- which do not actually support the hardware via that
  method.

This seems ripe to start to get really messy, really quickly.  Shouldn't
"default configs" handle if this "should" be enabled for a platform or
not, and let the rest of us just build them with no problems?

If every time a new Kconfig option is added, corresponding default
config updates come with it, sure. I just don't see that happening,
especially when it can be done much more clearly in the Kconfig while
the developer is writing the driver.

quoted

What problems is this causing you?  Are you running out of space in
kernel packages with drivers that will never be actually used?

Wasted build resources. Wasted disk space on /every/ system the kernel
package is installed on. We're all trying to pare down the kernel
packages to eliminate wasted space and doing it manually means a bunch
of research, sometimes with incorrect assumptions about the results,
needs to be done by someone not usually associated with that code. That
research gets repeated by people maintaining kernel packages for pretty
much every distro.

I second all the above.

quoted

quoted

quoted

+config COMPILE_TEST
+	bool "Compile also drivers which will not load" if EXPERT

EXPERT is getting to be the "let's hide it here" option, isn't it...

I don't know, if no one else strongly objects, I can be convinced that
this is needed, but so far, I don't see why it really is, or what this
is going to help with.

I'm not convinced adding a || COMPILE_TEST option to every driver that
may be arch specific is the best way to go either. Perhaps adding a new
Kconfig verb called "archdepends on" or something that will evaluate as
true if COMPILE_TEST is enabled but will evaluate the conditional if
not. *waves hands*

Sam Ravnborg (the kconfig ex-maintainer) once wrote that he doesn't want
to extend the kconfig language for this purpose (which I support). That
a config option is fine and sufficient in this case [1]. Except he
called the config option "SHOW_ALL_DRIVERS". Adding the current
maintainer to CCs ;).

I agree with Sam. 'depends on XY || COMPILE_TEST' is quite
self-explanatory. And even if it's not, you can look up the help text
for COMPILE_TEST. With "archdepends on" or "available on", you need to
know what to look for to override the dependency.

you will still end up with:

depends on (ARCH_OMAP || ARCH_EXYNOS || ARCH_DAVINCI || ARCH_PPC || ...)

And every now and again that particular line will be updated to add
another arch dependency.

-- 
balbi