[PATCH 0/2] iw: fixes to Android.mk to include "iw" in AOSP builds

[PATCH 0/2] iw: fixes to Android.mk to include "iw" in AOSP builds

[Filipe Brandenburger]

Hi,

I'm working on including "iw" on Android AOSP eng builds and it turns out we
need some fixes to Android.mk to make it build on a current AOSP tree.

We already have an Android git repo set up with the "iw" tree:
https://android.googlesource.com/platform/external/iw.git

The two patches in this patchset will fix the references to libnl and remove
the nla_put_flag that might have been necessary in the past but is no longer
needed.

One alternative is to remove Android.mk from the upstream repo at kernel.org,
in which case we could simply maintain it downstream in the Android repo
instead.  If that's what you prefer, let me know and I can send you an
alternative commit to just remove those files.

Let me know if you have questions or suggestions.

Cheers,
Filipe


Filipe Brandenburger (2):
  iw: fix references to libnl in Android.mk
  iw: remove android-nl.c with unneeded workaround

 Android.mk   | 6 +++---
 android-nl.c | 6 ------
 2 files changed, 3 insertions(+), 9 deletions(-)
 delete mode 100644 android-nl.c

-- 
2.5.0.rc2.392.g76e840b

[PATCH 1/2] iw: fix references to libnl in Android.mk

[Filipe Brandenburger]

The latest AOSP refers to that library as libnl and not libnl_2.

TEST=Built AOSP tree with this patchset, tested the generated iw binary.

Signed-off-by: Filipe Brandenburger <filbranden@google.com>
---
 Android.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Android.mk b/Android.mk
index 735b236809ef..0820892c6c19 100644
--- a/Android.mk
+++ b/Android.mk
@@ -11,13 +11,13 @@ LOCAL_SRC_FILES := $(patsubst %.o,%.c,$(OBJS)) android-nl.c
 
 LOCAL_C_INCLUDES := \
 	$(LOCAL_PATH) \
-	external/libnl-headers/
+	external/libnl/include
 
 LOCAL_CFLAGS += -DCONFIG_LIBNL20
 LOCAL_LDFLAGS := -Wl,--no-gc-sections
 #LOCAL_MODULE_TAGS := optional
 LOCAL_MODULE_TAGS := eng
-LOCAL_STATIC_LIBRARIES := libnl_2
+LOCAL_STATIC_LIBRARIES := libnl
 LOCAL_MODULE := iw
 
 $(IW_SOURCE_DIR)/version.c:
-- 
2.5.0.rc2.392.g76e840b

[PATCH 2/2] iw: remove android-nl.c with unneeded workaround

[Filipe Brandenburger]

The workaround might have been necessary in the past, however now it
produces the following error:

  .../libnl.a(attr.o): multiple definition of 'nla_put_flag'
  .../android-nl.o: previous definition here
  collect2: error: ld returned 1 exit status

TEST=Built AOSP tree with this patchset, tested the generated iw binary.

Signed-off-by: Filipe Brandenburger <filbranden@google.com>
---
 Android.mk   | 2 +-
 android-nl.c | 6 ------
 2 files changed, 1 insertion(+), 7 deletions(-)
 delete mode 100644 android-nl.c

diff --git a/Android.mk b/Android.mk
index 0820892c6c19..8d6567f624c1 100644
--- a/Android.mk
+++ b/Android.mk
@@ -7,7 +7,7 @@ IW_ANDROID_BUILD=y
 NO_PKG_CONFIG=y
 include $(LOCAL_PATH)/Makefile
 
-LOCAL_SRC_FILES := $(patsubst %.o,%.c,$(OBJS)) android-nl.c
+LOCAL_SRC_FILES := $(patsubst %.o,%.c,$(OBJS))
 
 LOCAL_C_INCLUDES := \
 	$(LOCAL_PATH) \
diff --git a/android-nl.c b/android-nl.c
deleted file mode 100644
index d216f5fe8680..000000000000
--- a/android-nl.c
+++ /dev/null
@@ -1,6 +0,0 @@
-#include <netlink/attr.h>
-
-int nla_put_flag(struct nl_msg *msg, int flag)
-{
-	return nla_put(msg, flag, 0, NULL);
-}
-- 
2.5.0.rc2.392.g76e840b

Re: [PATCH 1/2] iw: fix references to libnl in Android.mk

[enh]

On Thu, Jul 30, 2015 at 2:35 PM, Filipe Brandenburger
<filbranden@google.com> wrote:
> The latest AOSP refers to that library as libnl and not libnl_2.
>
> TEST=Built AOSP tree with this patchset, tested the generated iw binary.
>
> Signed-off-by: Filipe Brandenburger <filbranden@google.com>
> ---
>  Android.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Android.mk b/Android.mk
> index 735b236809ef..0820892c6c19 100644
> --- a/Android.mk
> +++ b/Android.mk
> @@ -11,13 +11,13 @@ LOCAL_SRC_FILES := $(patsubst %.o,%.c,$(OBJS)) android-nl.c
>
>  LOCAL_C_INCLUDES := \
>         $(LOCAL_PATH) \
> -       external/libnl-headers/
> +       external/libnl/include

i think you can just remove the whole LOCAL_C_INCLUDES section; libnl
exports its headers. (sorry for not noticing this earlier.)

>  LOCAL_CFLAGS += -DCONFIG_LIBNL20
>  LOCAL_LDFLAGS := -Wl,--no-gc-sections
>  #LOCAL_MODULE_TAGS := optional
>  LOCAL_MODULE_TAGS := eng
> -LOCAL_STATIC_LIBRARIES := libnl_2
> +LOCAL_STATIC_LIBRARIES := libnl
>  LOCAL_MODULE := iw
>
>  $(IW_SOURCE_DIR)/version.c:
> --
> 2.5.0.rc2.392.g76e840b
>



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.

[PATCH v2 1/2] iw: fix references to libnl in Android.mk

[Filipe Brandenburger]

The latest AOSP refers to that library as libnl and not libnl_2.

TEST=Built AOSP tree with this patchset, tested the generated iw binary.

Signed-off-by: Filipe Brandenburger <filbranden@google.com>
---
v2: Removed now redundant LOCAL_C_INCLUDES as suggested by Elliott.
    Retested to confirm it still builds and works as intended.

 Android.mk | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/Android.mk b/Android.mk
index 735b236809ef..03bcc3e93d4d 100644
--- a/Android.mk
+++ b/Android.mk
@@ -9,15 +9,11 @@ include $(LOCAL_PATH)/Makefile
 
 LOCAL_SRC_FILES := $(patsubst %.o,%.c,$(OBJS)) android-nl.c
 
-LOCAL_C_INCLUDES := \
-	$(LOCAL_PATH) \
-	external/libnl-headers/
-
 LOCAL_CFLAGS += -DCONFIG_LIBNL20
 LOCAL_LDFLAGS := -Wl,--no-gc-sections
 #LOCAL_MODULE_TAGS := optional
 LOCAL_MODULE_TAGS := eng
-LOCAL_STATIC_LIBRARIES := libnl_2
+LOCAL_STATIC_LIBRARIES := libnl
 LOCAL_MODULE := iw
 
 $(IW_SOURCE_DIR)/version.c:
-- 
2.5.0.rc2.392.g76e840b

Re: [PATCH v2 1/2] iw: fix references to libnl in Android.mk

[Arik Nemtsov]

Won't this break iw build with earlier Android trees? Did you test
this on something like KK?

On Fri, Jul 31, 2015 at 12:47 AM, Filipe Brandenburger
<filbranden@google.com> wrote:
> The latest AOSP refers to that library as libnl and not libnl_2.
>
> TEST=Built AOSP tree with this patchset, tested the generated iw binary.
>
> Signed-off-by: Filipe Brandenburger <filbranden@google.com>
> ---
> v2: Removed now redundant LOCAL_C_INCLUDES as suggested by Elliott.
>     Retested to confirm it still builds and works as intended.
>
>  Android.mk | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/Android.mk b/Android.mk
> index 735b236809ef..03bcc3e93d4d 100644
> --- a/Android.mk
> +++ b/Android.mk
> @@ -9,15 +9,11 @@ include $(LOCAL_PATH)/Makefile
>
>  LOCAL_SRC_FILES := $(patsubst %.o,%.c,$(OBJS)) android-nl.c
>
> -LOCAL_C_INCLUDES := \
> -       $(LOCAL_PATH) \
> -       external/libnl-headers/
> -
>  LOCAL_CFLAGS += -DCONFIG_LIBNL20
>  LOCAL_LDFLAGS := -Wl,--no-gc-sections
>  #LOCAL_MODULE_TAGS := optional
>  LOCAL_MODULE_TAGS := eng
> -LOCAL_STATIC_LIBRARIES := libnl_2
> +LOCAL_STATIC_LIBRARIES := libnl
>  LOCAL_MODULE := iw
>
>  $(IW_SOURCE_DIR)/version.c:
> --
> 2.5.0.rc2.392.g76e840b
>

Re: [PATCH 2/2] iw: remove android-nl.c with unneeded workaround

[Arik Nemtsov]

On Fri, Jul 31, 2015 at 12:35 AM, Filipe Brandenburger
<filbranden@google.com> wrote:
> The workaround might have been necessary in the past, however now it
> produces the following error:
>
>   .../libnl.a(attr.o): multiple definition of 'nla_put_flag'
>   .../android-nl.o: previous definition here
>   collect2: error: ld returned 1 exit status

Since this is required by earlier versions, can't you #ifdef the code
according to version? Also for the makefile change..

Re: [PATCH 2/2] iw: remove android-nl.c with unneeded workaround

[enh]

no, because this is meant for the platform build system rather than
the NDK. although the NDK has a concept of target API level, the
platform only has "current".

On Thu, Jul 30, 2015 at 11:56 PM, Arik Nemtsov <arik@wizery.com> wrote:
> On Fri, Jul 31, 2015 at 12:35 AM, Filipe Brandenburger
> <filbranden@google.com> wrote:
>> The workaround might have been necessary in the past, however now it
>> produces the following error:
>>
>>   .../libnl.a(attr.o): multiple definition of 'nla_put_flag'
>>   .../android-nl.o: previous definition here
>>   collect2: error: ld returned 1 exit status
>
> Since this is required by earlier versions, can't you #ifdef the code
> according to version? Also for the makefile change..



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.

Re: [PATCH 2/2] iw: remove android-nl.c with unneeded workaround

[Arik Nemtsov]

On Fri, Jul 31, 2015 at 7:01 PM, enh <enh@google.com> wrote:
> no, because this is meant for the platform build system rather than
> the NDK. although the NDK has a concept of target API level, the
> platform only has "current".

Don't you have PLATFORM_VERSION?
http://androidxref.com/5.1.1_r6/xref/build/core/version_defaults.mk

And I see it's already used in some places.

My 2c is that it's a bad idea to break older version compatibility
when Android is not the owner of the project/git. Basically you don't
really have the same level of control over where this is used.

Arik

Re: [PATCH 2/2] iw: remove android-nl.c with unneeded workaround

[enh]

On Sat, Aug 1, 2015 at 11:57 PM, Arik Nemtsov <arik@wizery.com> wrote:
> On Fri, Jul 31, 2015 at 7:01 PM, enh <enh@google.com> wrote:
>> no, because this is meant for the platform build system rather than
>> the NDK. although the NDK has a concept of target API level, the
>> platform only has "current".
>
> Don't you have PLATFORM_VERSION?
> http://androidxref.com/5.1.1_r6/xref/build/core/version_defaults.mk

yes, but that's an arbitrary string.

> And I see it's already used in some places.

by OEM code that's expected to be obsolete anyway. if you only have to
list _past_ versions (and you only care about Google's Android, and
not, say, the Amazon fork), you can use PLATFORM_VERSION. but it
doesn't seem a good idea for general use.

> My 2c is that it's a bad idea to break older version compatibility
> when Android is not the owner of the project/git. Basically you don't
> really have the same level of control over where this is used.

my 2c is that it's a bad idea to have an Android.mk that doesn't work
right now. and because libnl isn't part of the platform API, there's
no correct version version check for you to use. some OEMs may have
have that function longer than others; some may still not have it.
saying that libnl 2.0 is a prerequisite and your project won't build
without it sounds perfectly reasonable to me.

if this is really the only thing preventing your project from working
with libnl_2 though, why don't we just add it?

-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.

Re: [PATCH 0/2] iw: fixes to Android.mk to include "iw" in AOSP builds

[Johannes Berg]

On Thu, 2015-07-30 at 14:35 -0700, Filipe Brandenburger wrote:
> Hi,
> 
> I'm working on including "iw" on Android AOSP eng builds and it turns 
> out we
> need some fixes to Android.mk to make it build on a current AOSP 
> tree.
> 

We have a very similar patch internally as well (that I was still
evualating/reviewing) but it was less complete, so I've applied this
despite Arik's objections.

I think that if anyone needs to build against older Android, which is
very well possible, we can partially revert this and maybe provide some
environment variable they can control from their BSP, but until
somebody really shows up with that I'll leave it as is and assume that
older Android builds will also use older iw.

johannes

Re: [PATCH 0/2] iw: fixes to Android.mk to include "iw" in AOSP builds

[Filipe Brandenburger]

Hi Johannes,

On Thu, Aug 13, 2015 at 2:01 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> We have a very similar patch internally as well (that I was still
> evualating/reviewing) but it was less complete, so I've applied this
> despite Arik's objections.

Thank you!

> I think that if anyone needs to build against older Android, which is
> very well possible, we can partially revert this and maybe provide some
> environment variable they can control from their BSP, but until
> somebody really shows up with that I'll leave it as is and assume that
> older Android builds will also use older iw.

Short term, yes, whoever wants to build this against KitKat can just
revert this locally.

Long term, considering this is in AOSP and is part of eng/userdebug
builds, it's likely to be automatically available in future versions
of Android.

We ended up having to tweak the Android.mk again in the AOSP tree,
since the current one is doing an "include" of the external Makefile,
which in turns overrides the default "clean" rule and leaks some
pattern rules such as %.o, etc. which affect the rest of Android
build. For more details, please take a look at this Gerrit:
https://android-review.googlesource.com/166104

I'm cc'ing Ying who spotted the problem and suggested the changes.

One problem with the current approach is that there's duplication of
the names of the source files between Android.mk and Makefile, I was
thinking perhaps we could introduce a common.mk or sources.mk that's
included from both and only has the variables that can be used by
both? Suggestions are welcome.

As mentioned in the initial thread, one option is to drop Android.mk
from upstream altogether and maintain it in our AOSP downstream.

Thanks!
Filipe

Re: [PATCH 0/2] iw: fixes to Android.mk to include "iw" in AOSP builds

[Johannes Berg]

On Thu, 2015-08-13 at 09:55 -0700, Filipe Brandenburger wrote:

> Long term, considering this is in AOSP and is part of eng/userdebug
> builds, it's likely to be automatically available in future versions
> of Android.

Yeah, but most vendors don't actually build/run/ship AOSP nor
necessarily an unmodified iw, so it's likely not all that helpful.

> We ended up having to tweak the Android.mk again in the AOSP tree,
> since the current one is doing an "include" of the external Makefile,
> which in turns overrides the default "clean" rule and leaks some
> pattern rules such as %.o, etc. which affect the rest of Android
> build. For more details, please take a look at this Gerrit:
> https://android-review.googlesource.com/166104
> 
> I'm cc'ing Ying who spotted the problem and suggested the changes.

Thanks, but I'm not going to apply that patch in my tree - I don't want
to have to worry about the duplication of LOCAL_SRC_FILES.

> One problem with the current approach is that there's duplication of
> the names of the source files between Android.mk and Makefile, I was
> thinking perhaps we could introduce a common.mk or sources.mk that's
> included from both and only has the variables that can be used by
> both? Suggestions are welcome.

That seems reasonable.

> As mentioned in the initial thread, one option is to drop Android.mk
> from upstream altogether and maintain it in our AOSP downstream.

Given what I just said above about vendor builds etc. not being AOSP,
that probably wouldn't be all that helpful; I'm pretty sure it'd make
our life more difficult so I don't really like that idea much.

johannes

Re: [PATCH 0/2] iw: fixes to Android.mk to include "iw" in AOSP builds

[enh]

On Thu, Aug 13, 2015 at 11:33 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2015-08-13 at 09:55 -0700, Filipe Brandenburger wrote:
>
>> Long term, considering this is in AOSP and is part of eng/userdebug
>> builds, it's likely to be automatically available in future versions
>> of Android.
>
> Yeah, but most vendors don't actually build/run/ship AOSP nor
> necessarily an unmodified iw, so it's likely not all that helpful.

i think you misunderstand what AOSP is. this code is currently in AOSP
master, and thus in internal master, and thus in a future release.

>> We ended up having to tweak the Android.mk again in the AOSP tree,
>> since the current one is doing an "include" of the external Makefile,
>> which in turns overrides the default "clean" rule and leaks some
>> pattern rules such as %.o, etc. which affect the rest of Android
>> build. For more details, please take a look at this Gerrit:
>> https://android-review.googlesource.com/166104
>>
>> I'm cc'ing Ying who spotted the problem and suggested the changes.
>
> Thanks, but I'm not going to apply that patch in my tree - I don't want
> to have to worry about the duplication of LOCAL_SRC_FILES.
>
>> One problem with the current approach is that there's duplication of
>> the names of the source files between Android.mk and Makefile, I was
>> thinking perhaps we could introduce a common.mk or sources.mk that's
>> included from both and only has the variables that can be used by
>> both? Suggestions are welcome.
>
> That seems reasonable.
>
>> As mentioned in the initial thread, one option is to drop Android.mk
>> from upstream altogether and maintain it in our AOSP downstream.
>
> Given what I just said above about vendor builds etc. not being AOSP,
> that probably wouldn't be all that helpful; I'm pretty sure it'd make
> our life more difficult so I don't really like that idea much.
>
> johannes



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.

Re: [PATCH 0/2] iw: fixes to Android.mk to include "iw" in AOSP builds

[Johannes Berg]

On Thu, 2015-08-13 at 12:48 -0700, enh wrote:
> 
> i think you misunderstand what AOSP is. this code is currently in 
> AOSP master, and thus in internal master, and thus in a future 
> release.

Great, but as I understand it, vendors are under no obligation to
actually ship that code on their devices.

If a vendor needs to have an iw that, for example, supports their own
vendor commands (which, as I understand from the wifihal etc., Google
is basically forcing vendors to have!), then the AOSP iw is useless and
an own version needs to be built.


johannes

Re: [PATCH 0/2] iw: fixes to Android.mk to include "iw" in AOSP builds

[enh]

On Thu, Aug 13, 2015 at 1:01 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2015-08-13 at 12:48 -0700, enh wrote:
>>
>> i think you misunderstand what AOSP is. this code is currently in
>> AOSP master, and thus in internal master, and thus in a future
>> release.
>
> Great, but as I understand it, vendors are under no obligation to
> actually ship that code on their devices.
>
> If a vendor needs to have an iw that, for example, supports their own
> vendor commands (which, as I understand from the wifihal etc., Google
> is basically forcing vendors to have!), then the AOSP iw is useless and
> an own version needs to be built.

but given that our iw is your iw anyway... i don't understand the
situation in which they'd be inconvenienced here?

> johannes



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.

Re: [PATCH 0/2] iw: fixes to Android.mk to include "iw" in AOSP builds

[Johannes Berg]

On Thu, 2015-08-13 at 13:44 -0700, enh wrote:

> but given that our iw is your iw anyway... i don't understand the
> situation in which they'd be inconvenienced here?
> 

But it's not. We can reasonably expect everyone who actually cares to
have local modifications to iw that they aren't publishing (and that I
wouldn't apply anyway since it'd be for vendor commands) - and they
need to build *that* iw, not "our" iw.

The fact that libwifihal more or less wants you to do vendor extensions
makes that all the more likely, since if you want to test those with iw
then you need a modified iw.

johannes