Some bug fixing, cleanup and shared file

[Thaodan]

This pr fixes some lighther bugs like quoting or legacy backtics and moves everything that can be shared between the two scripts into a shared file.

[oshmoun]

while you're at it, it would be invaluable to have the actual build script seperate from the iteration over the devices to build. that way one has a simple script to build any device of their choice.

[MarijnS95]

@Thaodan Thanks for PR-ing this! I was about to ask in #4 whether to fix-up my commits and PR those, looks like you already took care of it.

I'll submit my cleaned-up (not yet pushed) patch tomorrow that takes care of the following (and rebased on this shared approach which is a really good idea :+1:):

• Build single devices
• To their own folder
• Does not delete output folder unless you ask for it (incremental builds, which are necessary for everyone without a ridiculous 56-core machine)

Pretty much what you just asked @oshmoun !

[Thaodan]

I wanted to the latter later because I already had so much cleanup done and add another PR for the rest. I don't think in their own folder build is needed. When building multiple times ccache should be used and when I build for one device I don't need to remove the folder before starting a new build.

[MarijnS95]

@Thaodan Yes, that is generally true. The build system will figure out what to rebuild and what to link into the final vmlinux image, so that is safe to do. Separate directories can be made optional, but I like it since disk space is more readily available on my build system in contrast to CPU speed (== lost time). Even with ccache it still has to go over each file, which takes almost a minute on my machine where a rebuild only takes 10-20 seconds, or something like that.

and add another PR for the rest.

Does that mean you will be submitting changes to do a per-device build easily? (Meaning both within an aosp environment as well as outside it, by eg. specifying the device as argument or env var)

Anyway, I heard @ix5 is looking into submitting the Android-10 compatible build scripts which could make this all a non-issue, as long as it doesn't involve too much hacking around the build system itself to get it to a somewhat user-friendly state.

[oshmoun]

one more suggestion (yes i know I'm just pestering at this point sorry!) but can logging be done with tee so that it still outputs to console in addition to a file?

[Thaodan]

It can and I thought of that too but I thought the point of that of output this wasn't intended.

[MartinX3]

Sadly I can't build tama on my Ubuntu 18.04 with your PR (It works with this https://github.com/MarijnS95/kernel-sony-msm-4.14-common/commits/aosp/LA.UM.7.1.r1 commits so my buildmachine is able to build the kernel and DTBO's)

Or maybe you need to rebase the PR, because I simply downloaded your branch and replaced the files on my computer with your files.

I also did remove the kernel-tmp folder before trying this PR.

=================================================
Your Environment:
ANDROID_ROOT: /home/developer/android/rom/sonyAOSP/10
KERNEL_TOP  : /home/developer/android/rom/sonyAOSP/10/kernel/sony/msm-4.14
KERNEL_TMP  : /home/developer/android/rom/sonyAOSP/10/out/kernel-tmp
=================================================
Platform -> tama :: Device -> akari
make[1]: Verzeichnis „/media/martin/extLinux/developer/android/cache/sonyAOSP/10/kernel-tmp“ wird betreten
  GEN     ./Makefile
  HOSTCC  scripts/basic/fixdep
  HOSTCC  scripts/kconfig/conf.o
  SHIPPED scripts/kconfig/zconf.tab.c
  SHIPPED scripts/kconfig/zconf.lex.c
  HOSTCC  scripts/kconfig/zconf.tab.o
  HOSTLD  scripts/kconfig/conf
#
# configuration written to .config
#
make[1]: Verzeichnis „/media/martin/extLinux/developer/android/cache/sonyAOSP/10/kernel-tmp“ wird verlassen
The build may take up to 10 minutes. Please be patient ...
Building new kernel image ...
Logging to /home/developer/android/rom/sonyAOSP/10/out/kernel-tmp/build_log_akari
Copying new kernel image ...
create image file: /home/developer/android/rom/sonyAOSP/10/kernel/sony/msm-4.14/common-kernel/dtbo-akari.img...
Can not read file: /home/developer/android/rom/sonyAOSP/10/out/kernel-tmp/arch/arm64/boot/dts/qcom/sdm845-v2.1-tama-akari_generic-overlay.dtbo
/home/developer/android/rom/sonyAOSP/10/out/kernel-tmp/arch/arm64/boot/dts/qcom/sdm845-tama-akari_generic-overlay.dtbo
/home/developer/android/rom/sonyAOSP/10/out/kernel-tmp/arch/arm64/boot/dts/qcom/sdm845-v2-tama-akari_generic-overlay.dtbo
Can not output image with args

The mentioned files do exist

ls -lha /home/developer/android/rom/sonyAOSP/10/out/kernel-tmp/arch/arm64/boot/dts/qcom/sdm845-v2.1-tama-akari_generic-overlay.dtbo
-rw-r--r-- 1 martin martin 291K Nov 22 18:05 /home/developer/android/rom/sonyAOSP/10/out/kernel-tmp/arch/arm64/boot/dts/qcom/sdm845-v2.1-tama-akari_generic-overlay.dtbo

ls -lha /home/developer/android/rom/sonyAOSP/10/out/kernel-tmp/arch/arm64/boot/dts/qcom/sdm845-tama-akari_generic-overlay.dtbo
-rw-r--r-- 1 martin martin 291K Nov 22 18:05 /home/developer/android/rom/sonyAOSP/10/out/kernel-tmp/arch/arm64/boot/dts/qcom/sdm845-tama-akari_generic-overlay.dtbo

ls -lha /home/developer/android/rom/sonyAOSP/10/out/kernel-tmp/arch/arm64/boot/dts/qcom/sdm845-v2-tama-akari_generic-overlay.dtbo
-rw-r--r-- 1 martin martin 291K Nov 22 18:05 /home/developer/android/rom/sonyAOSP/10/out/kernel-tmp/arch/arm64/boot/dts/qcom/sdm845-v2-tama-akari_generic-overlay.dtbo

[Thaodan]

Please read the log files there's a build error probably.

[MartinX3]

I don't see a build error in the build_log_akari build_log_akari.txt The kernel also got created and is 14.7 mb big.

Is there another build log in kernel-tmp I don't see @Thaodan?

And you can build the tama platform correctly with your PR?

And I was able to build the tama kernels before, only with the Marijn commits in his repository.

[MarijnS95]

Hi @thaodan

I'm a fan of this PR but unfortunately it looks like it has stalled. I've taken the liberty to revive it, including the following changes:

• Apply review comments (deduplicating the clang script)
• Ensuring ANDROID_ROOT is set in all clang.sh and gcc.sh
• Rebase on top of sonyxperiadev:aosp/LA.UM.7.1.r1 to get rid of the conflicts
• Compile all device kernels to their own directory
• Only delete output directories when CLEAN_AFTER_BUILD=true (parent dirs are not deleted, not sure if we want to do that at the end. That would delete all device dirs even when the script is edited to build a single device)
• Show make output directly on screen, except when LOG_TO_FILE=true
• Allow BUILD_ARGS to contain a path with spaces, generally used for CROSS_COMPILE="ccache gcc"
• Shellcheck warnings are a thing of the past now

You can find it here: https://github.com/MarijnS95/kernel-sony-msm-4.14-common/commits/massive-cleanup

Do you want to take over my branch (except the KEEPLOCAL commits, those are just for example), eventually amend some and update this PR, or shall I PR it?

After this we can look into building single devices/platforms like in #4. Though I would prefer ./build-kernels-gcc.sh griffin instead of DEVICE=griffin PLATFORM(s)=kumano ./build-kernels-gcc.sh but that's a discussion for the other PR :)

[Thaodan]

Hey

I'll take most of the commits but I don't think its nessary to a dir per compiler and device unless you want to support parallel builds. The output directory shouldn't be deleted to increase the speeds of rebuilds when testing changes.

Abou the situatiobn when it comes to logging: it depends on what should be the default. Currently the default was that logs didn't appear on screen. I'd rather use command line arguments to direct the output of those outputs or to set options in general when it comes to that matter.

[jerpelea]

please do cleanup by default if you build all kernels it is a huge waste of space

[MarijnS95]

I'll take most of the commits but I don't think its nessary to a dir per compiler and device unless you want to support parallel builds.

I generally do parallel builds with both compilers so that's a necessity for me when preserving the output (incremental build speed...)

The output directory shouldn't be deleted to increase the speeds of rebuilds when testing changes.

We'll keep that option for Alin though, who prefers (understandably) clean builds. He has an insane build machine after all, and can run this safely in the background without too much time constaints.

Abou the situatiobn when it comes to logging: it depends on what should be the default. Currently the default was that logs didn't appear on screen.

I personally prefer to have it on by default. If you don't notice that the command terminated or are testing locally, having the error pop up on the screen directly is invaluable. @jerpelea I think you use this to check build outputs for separate devices after the fact, when no file popped up?

I'd rather use command line arguments to direct the output of those outputs or to set options in general when it comes to that matter.

Like a filename? Sure, we can do that. Same for the delete-after-build I guess.

[MarijnS95]

@Thaodan We'll have to remove the " around "$(find "$KERNEL_TMP"/arch/arm64/boot/dts -name "*.dtbo")" since there are devices like Akatsuki with multiple DTBO files. This causes them to be one large filename with spaces, which is obviously not correct.

We could add something like -printf "\"%p\" " (note the space after the \") or do you have a better idea?

[Thaodan]

Find -print0 could work better.

[MarijnS95]

Find -print0 could work better.

Unfortunately that does not address the issue:

$ ls                                
'foo bar.dtbo'   'second file.dtbo'
$ ls $(find -iname '*.dtbo' -print0)  
ls: cannot access './foo': No such file or directory
ls: cannot access 'bar.dtbo': No such file or directory
ls: cannot access './second': No such file or directory
ls: cannot access 'file.dtbo': No such file or directory
ls: cannot access '': No such file or directory
$ ls "$(find -iname '*.dtbo' -print0)"
'./foo bar.dtbo'

[jerpelea]

please rebase

[Thaodan]

Hey,

I rebased the branch and added changes made by @MarijnS95 or took inspiration from him. Also I added the feature requested by @oshmoun to build for a single device by using -d as an argument. The build dir is only kept when using -k or building for a single device.

[Thaodan]

About the issue when having dtbo files with spaces, please see my last commit. The issue was the was no word splitting for the output of find.

[pablomh]

Please, consider dropping the "$VARIABLE"/foo and adopt the ${VARIABLE}/foo one (it makes my eyes bleed :( ).

[Thaodan]

{VAR}/foo doesn't do quoting the first one does.

[MarijnS95]

Pity you didn't manage to take anything from my branch. Means the time I spent amending fixes to (my own) commits, checking shellcheck and writing proper commit messages/descriptions splitting the features... has all gone to waste.

About the issue when having dtbo files with spaces, please see my last commit. The issue was the was no word splitting for the output of find.

We want word splitting between the files that find emits; in my opinion shellcheck warns for splitting between file paths, since find doesn't emit anything properly quoted/escaped on its own.

Code review follows inline... after cooking dinner.

[Thaodan]

Pity you didn't manage to take anything from my branch. Means the time I spent amending fixes to (my own) commits, checking shellcheck and writing proper commit messages/descriptions splitting the features... has all gone to waste. I did but didn't take the exact commits as I want to make these changes in a different order or way. You had for example had two changes in some commits, one that was explained in the commit and one that wasn't. I added credit to every change I have taken from your branch to make this clear.

Shellcheck throws a (yellow) warning in this case as you will the in my screenshot in the next message.

[Thaodan]

[MarijnS95]

I did but didn't take the exact commits as I want to make these changes in a different order or way.

IMO it's easier to leave commits out and amend small changes to others; that way attribution is preserved just like I did with your commits. Fair though, maybe I'm a bit too pedantic when it comes to maintaining authorship for such small changes? More importantly I'm missing the cleanups that I amended to my own messy and not upstreamable commits, and cleanups to the general script and/or amended to yours.

You had for example had two changes in some commits, one that was explained in the commit and one that wasn't. I added credit to every change I have taken from your branch to make this clear.

Interesting, I took (extra) care to keep it clean and have every change separated. Guess it's Build all device kernels to their own directory while also adding the option to clean after build (which was/is inherently intertwined with device separation)?

[jerpelea]

please update the PR

[MartinX3]

Please rebase

[jerpelea]

@Thaodan please rebase

[Thaodan]

@jerpelea Will do that today.

[Thaodan]

Done.