Re: [PATCH] drm/i915: add Makefile magic for testing headers are self-contained

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 03 Apr 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> From: Jani Nikula <jani.nikula@intel.com>
>
> The below commits added dummy files to test that certain headers are
> self-contained, i.e. compilable as standalone units:
>
> 39e2f501c1b4 ("drm/i915: Split struct intel_context definition to its own header")
> 3a891a626794 ("drm/i915: Move intel_engine_mask_t around for use by i915_request_types.h")
> 8b74594aa455 ("drm/i915: Split out i915_priolist_types into its own header")
>
> The idea is fine, but the implementation is a bit tedious and
> inflexible, and does not really scale well.
>
> Implement the same in make using autogenerated dummy sources to include
> the headers.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> Use patsubst liberally to reduce the chore of adding more headers.
> make clean doesn't quite work yet, although it seems to be the right
> style.

I don't know what gives, I'm unable to make it work. I've been trying
with O= builds.

> ---
>  drivers/gpu/drm/i915/.gitignore               |  1 +
>  drivers/gpu/drm/i915/Makefile                 | 16 ++++---------
>  drivers/gpu/drm/i915/Makefile.header-test     | 24 +++++++++++++++++++
>  .../i915/test_i915_active_types_standalone.c  |  7 ------
>  .../test_i915_gem_context_types_standalone.c  |  7 ------
>  .../test_i915_priolist_types_standalone.c     |  7 ------
>  .../test_i915_scheduler_types_standalone.c    |  7 ------
>  .../test_i915_timeline_types_standalone.c     |  7 ------
>  .../test_intel_context_types_standalone.c     |  7 ------
>  .../i915/test_intel_engine_types_standalone.c |  7 ------
>  .../test_intel_workarounds_types_standalone.c |  7 ------
>  11 files changed, 30 insertions(+), 67 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/.gitignore
>  create mode 100644 drivers/gpu/drm/i915/Makefile.header-test
>  delete mode 100644 drivers/gpu/drm/i915/test_i915_active_types_standalone.c
>  delete mode 100644 drivers/gpu/drm/i915/test_i915_gem_context_types_standalone.c
>  delete mode 100644 drivers/gpu/drm/i915/test_i915_priolist_types_standalone.c
>  delete mode 100644 drivers/gpu/drm/i915/test_i915_scheduler_types_standalone.c
>  delete mode 100644 drivers/gpu/drm/i915/test_i915_timeline_types_standalone.c
>  delete mode 100644 drivers/gpu/drm/i915/test_intel_context_types_standalone.c
>  delete mode 100644 drivers/gpu/drm/i915/test_intel_engine_types_standalone.c
>  delete mode 100644 drivers/gpu/drm/i915/test_intel_workarounds_types_standalone.c
>
> diff --git a/drivers/gpu/drm/i915/.gitignore b/drivers/gpu/drm/i915/.gitignore
> new file mode 100644
> index 000000000000..cff45d81f42f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/.gitignore
> @@ -0,0 +1 @@
> +header_test_*.c
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 30bf3301ea24..91eaccca4f4f 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -32,6 +32,11 @@ CFLAGS_intel_fbdev.o = $(call cc-disable-warning, override-init)
>  subdir-ccflags-y += \
>  	$(call as-instr,movntdqa (%eax)$(comma)%xmm0,-DCONFIG_AS_MOVNTDQA)
>  
> +# Extra header tests
> +ifeq ($(CONFIG_DRM_I915_WERROR),y)
> +include $(src)/Makefile.header-test
> +endif
> +

Agreed on adding this first, though you'll need to omit -j to actually
make it happen first.

>  # Please keep these build lists sorted!
>  
>  # core driver code
> @@ -57,17 +62,6 @@ i915-$(CONFIG_COMPAT)   += i915_ioc32.o
>  i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o intel_pipe_crc.o
>  i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o
>  
> -# Test the headers are compilable as standalone units
> -i915-$(CONFIG_DRM_I915_WERROR) += \
> -	test_i915_active_types_standalone.o \
> -	test_i915_gem_context_types_standalone.o \
> -	test_i915_priolist_types_standalone.o \
> -	test_i915_scheduler_types_standalone.o \
> -	test_i915_timeline_types_standalone.o \
> -	test_intel_context_types_standalone.o \
> -	test_intel_engine_types_standalone.o \
> -	test_intel_workarounds_types_standalone.o
> -
>  # GEM code
>  i915-y += \
>  	  i915_active.o \
> diff --git a/drivers/gpu/drm/i915/Makefile.header-test b/drivers/gpu/drm/i915/Makefile.header-test
> new file mode 100644
> index 000000000000..5396bd6bae4c
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/Makefile.header-test
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: MIT
> +# Copyright © 2019 Intel Corporation
> +
> +# Test the headers are compilable as standalone units
> +header_test := \
> +	i915_active_types.h \
> +	i915_gem_context_types.h \
> +	i915_priolist_types.h \
> +	i915_scheduler_types.h \
> +	i915_timeline_types.h \
> +	intel_context_types.h \
> +	intel_engine_types.h \
> +	intel_workarounds_types.h
> +
> +quiet_cmd_header_test = HDRTEST	$@
> +      cmd_header_test = echo "\#include \"$(<F)\"" > $@
> +
> +header_test_%.c: %.h
> +	$(call cmd,header_test)
> +
> +# To test <base>.h add header_test_<base>.o here

The comment just became stale.

> +extra-y += $(foreach h,$(header_test),$(patsubst %.h,header_test_%.o,$(h)))

Nice.

For making subdirs work, I think the rules should be separate from
declaring the headers to test, i.e. each subdir needs to have a Makefile
that's included using "obj-y += subdir/" in the parent instead of
"include $(src)/subdir/Makefile". Ideally, the rules would be in the
global Makefile.build... using something like "headertest-y". But let's
not get ahead of ourselves.

> +
> +clean-files += $(foreach h,$(header_test),$(patsubst %.h,header_test_%.c,$(h)))
> diff --git a/drivers/gpu/drm/i915/test_i915_active_types_standalone.c b/drivers/gpu/drm/i915/test_i915_active_types_standalone.c
> deleted file mode 100644
> index 144ebd153e57..000000000000
> --- a/drivers/gpu/drm/i915/test_i915_active_types_standalone.c
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -/*
> - * SPDX-License-Identifier: MIT
> - *
> - * Copyright © 2019 Intel Corporation
> - */
> -
> -#include "i915_active_types.h"
> diff --git a/drivers/gpu/drm/i915/test_i915_gem_context_types_standalone.c b/drivers/gpu/drm/i915/test_i915_gem_context_types_standalone.c
> deleted file mode 100644
> index 4e4da4860bc2..000000000000
> --- a/drivers/gpu/drm/i915/test_i915_gem_context_types_standalone.c
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -/*
> - * SPDX-License-Identifier: MIT
> - *
> - * Copyright © 2019 Intel Corporation
> - */
> -
> -#include "i915_gem_context_types.h"
> diff --git a/drivers/gpu/drm/i915/test_i915_priolist_types_standalone.c b/drivers/gpu/drm/i915/test_i915_priolist_types_standalone.c
> deleted file mode 100644
> index f465eb99bb47..000000000000
> --- a/drivers/gpu/drm/i915/test_i915_priolist_types_standalone.c
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -/*
> - * SPDX-License-Identifier: MIT
> - *
> - * Copyright © 2019 Intel Corporation
> - */
> -
> -#include "i915_priolist_types.h"
> diff --git a/drivers/gpu/drm/i915/test_i915_scheduler_types_standalone.c b/drivers/gpu/drm/i915/test_i915_scheduler_types_standalone.c
> deleted file mode 100644
> index 8afa2c3719fb..000000000000
> --- a/drivers/gpu/drm/i915/test_i915_scheduler_types_standalone.c
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -/*
> - * SPDX-License-Identifier: MIT
> - *
> - * Copyright © 2019 Intel Corporation
> - */
> -
> -#include "i915_scheduler_types.h"
> diff --git a/drivers/gpu/drm/i915/test_i915_timeline_types_standalone.c b/drivers/gpu/drm/i915/test_i915_timeline_types_standalone.c
> deleted file mode 100644
> index f58e148e8946..000000000000
> --- a/drivers/gpu/drm/i915/test_i915_timeline_types_standalone.c
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -/*
> - * SPDX-License-Identifier: MIT
> - *
> - * Copyright © 2019 Intel Corporation
> - */
> -
> -#include "i915_timeline_types.h"
> diff --git a/drivers/gpu/drm/i915/test_intel_context_types_standalone.c b/drivers/gpu/drm/i915/test_intel_context_types_standalone.c
> deleted file mode 100644
> index b39e3c4e6551..000000000000
> --- a/drivers/gpu/drm/i915/test_intel_context_types_standalone.c
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -/*
> - * SPDX-License-Identifier: MIT
> - *
> - * Copyright © 2019 Intel Corporation
> - */
> -
> -#include "intel_context_types.h"
> diff --git a/drivers/gpu/drm/i915/test_intel_engine_types_standalone.c b/drivers/gpu/drm/i915/test_intel_engine_types_standalone.c
> deleted file mode 100644
> index d05e4cdcbcf9..000000000000
> --- a/drivers/gpu/drm/i915/test_intel_engine_types_standalone.c
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -/*
> - * SPDX-License-Identifier: MIT
> - *
> - * Copyright © 2019 Intel Corporation
> - */
> -
> -#include "intel_engine_types.h"
> diff --git a/drivers/gpu/drm/i915/test_intel_workarounds_types_standalone.c b/drivers/gpu/drm/i915/test_intel_workarounds_types_standalone.c
> deleted file mode 100644
> index 4f658bb00825..000000000000
> --- a/drivers/gpu/drm/i915/test_intel_workarounds_types_standalone.c
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -/*
> - * SPDX-License-Identifier: MIT
> - *
> - * Copyright © 2019 Intel Corporation
> - */
> -
> -#include "intel_workarounds_types.h"

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx