Re: [PATCH libdrm] intel: annotate the intel genx helpers as private

[Eric Engestrom <eric.engestrom@intel.com>]

On Thursday, 2018-09-13 10:43:04 -0700, Rodrigo Vivi wrote:
> On Thu, Sep 13, 2018 at 09:45:47AM +0100, Eric Engestrom wrote:
> > On Thursday, 2018-09-13 15:19:00 +0800, Chih-Wei Huang wrote:
> > > Note this patch breaks drm_gralloc:
> > > 
> > > FAILED: out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/LINKED/libgralloc_drm.so
> > > /bin/bash -c "prebuilts/clang/host/linux-x86/clang-4053586/bin/clang++
> > > -nostdlib -Wl,-soname,libgralloc_drm.so -Wl,--gc-sections -shared
> > > out/target/product/x86_64/obj_x86/lib/crtbegin_so.o
> > > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/gralloc_drm.o
> > > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/gralloc_drm_kms.o
> > > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/gralloc_drm_intel.o
> > > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/gralloc_drm_radeon.o
> > > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/gralloc_drm_nouveau.o
> > > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/gralloc_drm_pipe.o
> > > -Wl,--whole-archive  -Wl,--no-whole-archive
> > > out/target/product/x86_64/obj_x86/STATIC_LIBRARIES/libcompiler_rt-extras_intermediates/libcompiler_rt-extras.a
> > >   out/target/product/x86_64/obj_x86/STATIC_LIBRARIES/libatomic_intermediates/libatomic.a
> > > out/target/product/x86_64/obj_x86/STATIC_LIBRARIES/libgcc_intermediates/libgcc.a
> > > -Wl,-z,noexecstack -Wl,-z,relro -Wl,-z,now -Wl,--build-id=md5
> > > -Wl,--warn-shared-textrel -Wl,--fatal-warnings -Wl,--gc-sections
> > > -Wl,--hash-style=gnu -Wl,--no-undefined-version -m32  -target
> > > i686-linux-android
> > > -Bprebuilts/gcc/linux-x86/x86/x86_64-linux-android-4.9/x86_64-linux-android/bin
> > > -Wl,--no-undefined out/target/product/x86_64/obj_x86/lib/libdrm.so
> > > out/target/product/x86_64/obj_x86/lib/liblog.so
> > > out/target/product/x86_64/obj_x86/lib/libcutils.so
> > > out/target/product/x86_64/obj_x86/lib/libhardware_legacy.so
> > > out/target/product/x86_64/obj_x86/lib/libdrm_intel.so
> > > out/target/product/x86_64/obj_x86/lib/libdrm_radeon.so
> > > out/target/product/x86_64/obj_x86/lib/libdrm_nouveau.so
> > > out/target/product/x86_64/obj_x86/lib/libc++.so
> > > out/target/product/x86_64/obj_x86/lib/libc.so
> > > out/target/product/x86_64/obj_x86/lib/libm.so
> > > out/target/product/x86_64/obj_x86/lib/libdl.so -o
> > > out/target/product/x86_64/obj_x86/SHARED_LIBRARIES/libgralloc_drm_intermediates/LINKED/libgralloc_drm.so
> > > out/target/product/x86_64/obj_x86/lib/crtend_so.o"
> > > external/drm_gralloc/gralloc_drm_intel.c:631: error: undefined
> > > reference to 'intel_is_genx'
> > > external/drm_gralloc/gralloc_drm_intel.c:630: error: undefined
> > > reference to 'intel_get_genx'
> > > clang.real: error: linker command failed with exit code 1 (use -v to
> > > see invocation)
> > > 
> > > 
> > > That's because drm_gralloc use the IS_GEN9 macro
> > > (and other IS_GEN{n} macros) directly.
> > 
> > Yeah, I thought some other stuff used the IS_GEN* macros ¯\_(ツ)_/¯
> > 
> > I assume my other patch ("intel: use drm namespace for exported functions",
> > https://patchwork.kernel.org/patch/10590653/) works fine with drm_gralloc?
> 
> What a castle of cards we have here....
> 
> We should pick one build system and support only this one build system.

Deprecating autotools in libdrm is a worthwhile discussion, but it
should probably be its own thread, otherwise most people will miss it.

As for Android.mk, it isn't going anywhere in the near future; it would
require AOSP to support meson in their build system, or someone to write
a compatibility layer. There's already a thread somewhere on mesa-dev@
about this.

> 
> And the default build all should include tests. We shouldn't need a web
> remote CI to inform us that we could be breaking the build.

The CI runs the same tests you have locally ;)

> 
> When I pushed the first series I build locally using autotools and meson
> and everything passed locally.

`meson test` wouldn't have passed, but there is a bug in the autotools
test scripts that Emil has identified that made them not actually run...

> 
> But then gitlab CI warned that ninja buildir test fail...
> 
> Than Daniel requested to use gitlab fork to make sure we pass CI
> and here I went:
> 
> local ninja -C builddir test was happy and CI was green as we can see here:
> https://gitlab.freedesktop.org/vivijim/drm/commit/b2c25182f29a0000b010afbb11748c213c9a3e8a/pipelines?ref=master
> 
> But then I just discovered that this broken autotools build!

This isn't a different build system, this is an external project that
uses libdrm (and depended on the macros that were rewritten).

> 
> > 
> > > 
> > > Since IS_GEN{n} are public APIs, I don't see
> > > the rationale of this change.
> > > Unless you are going to privatize all IS_GEN{n} macros.
> > > (are you?)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel