[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/android-port-new into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: hipl-dev@xxxxxxxxxxxxx
- Date: Fri, 1 Nov 2013 21:39:13 +0100
On Fri, Nov 01, 2013 at 01:04:42PM +0200, Juhani Toivonen wrote:
> On 31.10.2013, at 18:43, Diego Biurrun <diego@xxxxxxxxxx> wrote:
> > On Thu, Oct 31, 2013 at 02:07:28PM -0000, Juhani Toivonen wrote:
> >> Juhani Toivonen has proposed merging lp:~hipl-core/hipl/android-port-new
> >> into lp:hipl.
> >>
> >> Requested reviews:
> >> Miika Komu (miika-iki)
> >>
> >> For more details, see:
> >> https://code.launchpad.net/~hipl-core/hipl/android-port-new/+merge/193420
> >
> > Did you install all local pre-commit hooks? This is still full of tabs.
>
> No. The hooks would not let commit if I disagree with the changes
> they propose, and I didn’t want them to touch the imported files
> under the android folder, so I disabled them and have been running
> uncrustify separately for other files.
Well, we need to decide what to do with those imported files...
If they will interfere with our hooks I think it's better to adapt them.
Or we can update the hooks to exempt a few files from checking.
Miika?
> >> --- Makefile.am 2013-10-17 23:06:20 +0000
> >> +++ Makefile.am 2013-10-31 14:03:43 +0000
> >> @@ -23,7 +23,7 @@
> >>
> >> -HIPL_HEADER_LOCATIONS = hipd/*.h hipfw/*.h libcore/*.h libcore/*/*.h
> >> libhipl/*.h modules/*/*/*.h test/*.h test/*/*.h test/*/*/*.h
> >> +HIPL_HEADER_LOCATIONS = android android/linux hipd/*.h hipfw/*.h
> >> libcore/*.h libcore/*/*.h libhipl/*.h modules/*/*/*.h test/*.h test/*/*.h
> >> test/*/*/*.h
> >> HIPL_HEADER_LIST = $(wildcard $(addprefix
> >> $(srcdir)/,$(HIPL_HEADER_LOCATIONS)))
> >
> > Notice how the pattern you add differs from the existing ones. This will
> > not work. Please run make check to see that no headers in those subdirs
> > are compiled.
>
> Ok, added "/*.h” to the end of each.
I changed Makefile.am to automatically append *.h instead.
> I see nothing about stuff under android/ in make check.
Does "make alltests" pass? And does "make checkheaders" compile
anything under android/?
> >> --- android/ifaddrs.c 1970-01-01 00:00:00 +0000
> >> +++ android/ifaddrs.c 2013-10-31 14:03:43 +0000
> >> @@ -0,0 +1,640 @@
> >> +++ android/ifaddrs.h 2013-10-31 14:03:43 +0000
> >
> > This header is bogus. It contains tabs, illegal identifiers (starting
> > with _ and capital letter), strange ifdefs and whatnot. If you make this
> > a local HIPL header, delete all the crap. Also, this is a standardized
> > public interface and not copyrightable. Just use the standard HIPL
> > license header.
> >
> >> +++ android/linux/xfrm.h 2013-10-31 14:03:43 +0000
> >
> > What is this thing full of tabs and without license header?
>
> The xfrm.h header file is from Ubuntu precise. It has been marked as
>one of the GPL headers in the COPYING document.
>
> I was planning to change the borrowed source files as little as I
> can.
In general this is a good strategy. We will have to balance it against
other considerations, however.
> That is why I have tried to avoid touching the copyright/license
> headers or the original coding style in the files. Especially with
> copyright/licence headers I’m not really sure in what ways is it
> even ok to change them. Removing an original license header and
> especially replacing it with our own doesn’t sound quite right in
> most cases.
In general yes, but nothing stops you from rolling your own header.
It's quite trivial and the interfaces are standardized.
> As for the ifaddrs.(c|h) files that you really seem to want to get rid
> of, they are apparently only used by libhipl/netdev.c for creating a
> NETLINK socket. If we can move to another way that doesn’t involve
> calls to getifaddrs() or freeifaddrs(), we can remove those files.
Upon investigating that a bit, eliminating those calls would be the far
better solution. If you look at the comments in the file you will see
that it is a giant hack anyway. Now you are adding more hacks to keep
the original one. Not a good longterm strategy ...
> >> --- configure.ac 2013-08-06 19:17:14 +0000
> >> +++ configure.ac 2013-10-31 14:03:43 +0000
> >> @@ -122,6 +122,23 @@
> >> +AC_ARG_ENABLE(android,
> >> + AS_HELP_STRING([--enable-android],
> >> + [Enable android (default is NO)]),
> >> + [ac_cv_use_android=$enableval],
> >> + [ac_cv_use_android=no])
> >> +AC_CACHE_CHECK([whether to build for Android],
> >> + [ac_cv_use_android],
> >> + [ac_cv_use_android=no])
> >> +if test x"$ac_cv_use_android" = x"yes"; then
> >> + AC_DEFINE(CONFIG_HIP_ANDROID)
> >> + AC_SUBST(AM_CFLAGS, "-std=gnu99 -mbionic -fPIC -fno-exceptions")
> >
> > Why do you need gnu99?
>
> We initially got it to compile much further with it;
> now it seems to work on c99 too
So drop the flag. It's dangerous to let GNUisms slip in.
> >> --- libcore/builder.c 2013-10-22 12:34:28 +0000
> >> +++ libcore/builder.c 2013-10-31 14:03:43 +0000
> >> @@ -126,18 +126,27 @@
> >> static void convert_byte_order(void *content, unsigned count,
> >> unsigned item_size, enum cbo_flags flag)
> >> {
> >> - uint32_t (*f32)(uint32_t) = (flag == CBO_HTON) ? htonl : ntohl;
> >> - uint16_t (*f16)(uint16_t) = (flag == CBO_HTON) ? htons : ntohs;
> >> -
> >> - if (item_size == sizeof(uint16_t)) {
> >> + if (sizeof(uint16_t) == item_size) {
> >> uint16_t *p = content;
> >> - for (unsigned i = 0; i < count; i++) {
> >> - p[i] = f16(p[i]);
> >> + if (CBO_HTON == flag) { // 16 bit host to network
> >> + for (unsigned i = 0; i < count; i++) {
> >> + p[i] = htons(p[i]);
> >> + }
> >> + } else { // 16 bit network to host
> >> + for (unsigned i = 0; i < count; i++) {
> >> + p[i] = ntohs(p[i]);
> >> + }
> >> }
> >> - } else if (item_size == sizeof(uint32_t)) {
> >> + } else {
> >> uint32_t *p = content;
> >> - for (unsigned i = 0; i < count; i++) {
> >> - p[i] = f32(p[i]);
> >> + if (CBO_HTON == flag) { // 32 bit host to network
> >> + for (unsigned i = 0; i < count; i++) {
> >> + p[i] = htonl(p[i]);
> >> + }
> >> + } else { // 32 bit network to host
> >> + for (unsigned i = 0; i < count; i++) {
> >> + p[i] = ntohl(p[i]);
> >> + }
> >> }
> >> }
> >> }
> >
> > Why? This is now three times at long, for what gain?
>
> Again, to get rid of the function pointers. NDK defines ntohl etc. as macros.
> Do you have a prettier way?
Never mind, I forgot about the macro issue.
> >> --- tools/prepare_android_toolchain.sh 1970-01-01 00:00:00 +0000
> >> +++ tools/prepare_android_toolchain.sh 2013-10-31 14:03:43 +0000
> >> @@ -0,0 +1,174 @@
> >> +# Script is for building an Android cross-compilation environment on
> >> linux.
> >> +
> >
> > What sort of script is this without shebang and executable permissions?
>
> You were previously asking if the script was bash-specific because of the
> #!/bin/bash
>
> I made changes for it to work on /bin/sh too. However the script calls other
> scripts.
> For example the script for making the standalone NDK toolchain prints a
> warning message
> if the interpreter is not /bin/bash. It says that they try to make it work
> everywhere
> but the safest choice is to run it in bash.
>
> Now in my script I can either say #!/bin/bash, which is nice and comfy, but
> introduces
> dependency on bash; or always run it without bash by saying #!/bin/sh but I’d
> need to
> trust the other scripts more than their writers do; or I can leave the whole
> #! out to
> opportunistically run it in bash if the user is using that or take the risk
> if they’re not.
>
> Do you want me to leave it like this or put the #!/bin/bash back?
Then bash is the better solution.
Diego
Other related posts:
