buildsystem: rework build script #2151

InuSasha · 2017-10-27T12:20:42Z

Done:

combine PKG_AUTORECONF, PKG_USE_MESON, PKG_USE_CMAKE, PKG_USE_NINJA in to one PKG_TOOLCHAIN
replace some cascading if-elif-else-fi blocks with more readable case-blocks
add manual toolchain, with only run configure, make and makeinstall if defined in package.mk
reduce same more cascaded if-blocks (stamp and virtual)

Open:

none yet

Test: 28. 10

build: Generic, RPi1/2, Odroid, WeTek H/C/P1/P2 like master (incl. addons)
run: none

comments?

MilhouseVH · 2017-10-27T13:07:41Z

scripts/build

+        elif [ -f $PKG_BUILD/Makefile ]; then
+          PKG_TOOLCHAIN="make"
+        else
+          echo "Not possible to detect toolchain automactcly. Add PKG_TOOLCHAIN= in to package.mk"


Typo + wording:

echo "Not possible to detect toolchain automatically. Add PKG_TOOLCHAIN= to package.mk"

... c++ is much more easier then english :)

MilhouseVH · 2017-10-27T13:27:45Z

scripts/build

+           $PKG_TOOLCHAIN != "manual" ]; then
+        printf "$(print_color bold-red "ERROR: ")unknown toolchain $PKG_TOOLCHAIN"
+        exit 1
+      fi


You could use...

if ! listcontains "meson cmake cmake-make configure make autotools manual" "$PKG_TOOLCHAIN"; then printf "$(print_color CLR_ERROR "ERROR: ")unknown toolchain $PKG_TOOLCHAIN" exit 1 fi

Also changed to use predefined CLR_ERROR for errors.

MilhouseVH · 2017-10-27T13:35:45Z

scripts/build

+        printf "$(print_color bold-red "ERROR: ")unknown toolchain $PKG_TOOLCHAIN"
+        exit 1
+      fi
+      printf "%${BUILD_INDENT}c $(print_color CLR_TOOLCHAIN "TOOLCHAIN    ")$PKG_TOOLCHAIN${_auto_toolchain}\n" ' '>&$SILENT_OUT


Nit - stylistically this is inconsistent with existing usage, eg.:

printf "%${BUILD_INDENT}c $(print_color CLR_UNPACK "UNPACK") $1\n" ' '>&$SILENT_OUT

The spaces follow the call to print_color, so your line should be:

- printf "%${BUILD_INDENT}c $(print_color CLR_TOOLCHAIN "TOOLCHAIN ")$PKG_TOOLCHAIN${_auto_toolchain}\n" ' '>&$SILENT_OUT + printf "%${BUILD_INDENT}c $(print_color CLR_TOOLCHAIN "TOOLCHAIN") $PKG_TOOLCHAIN${_auto_toolchain}\n" ' '>&$SILENT_OUT

Didn't want to approve just yet!

MilhouseVH · 2017-10-27T13:39:37Z

Looks like a generally positive change... to avoid constant rebasing this needs to go in sooner than later. I'll run some local builds.

InuSasha · 2017-10-27T13:43:09Z

@MilhouseVH will fix you comments, add finish my open todos, this evening (CEST).

MilhouseVH · 2017-10-27T16:58:18Z

scripts/build

+  "target") _pkg_depends="$PKG_DEPENDS_TARGET";;
+  "host") _pkg_depends="$PKG_DEPENDS_HOST";;
+  "init") _pkg_depends="$PKG_DEPENDS_INIT";;
+  "bootstrap") _pkg_depends="$PKG_DEPENDS_BOOTSTRAP";;


I'd prefer a catch-all here:

*) _pkg_depends=;;

or initialise _pkg_depends= (or unset _pkg_depends) before the case. It shouldn't be set to anything, of course, but who knows what variables will be set when sourcing other files - the package.mk is sourced shortly before this section and it can set anything it likes.

I'd even be inclined to wrap the for loop in a test, just for clarity:

if [ -n "$_pkg_depends" ]; then <for loop> fi

bash will drop straight through the loop when _pkg_depends is blank or unset, but it avoids any confusion...

I am not sure about the the extra if. I do not like extra code.
The unset is added, will commit later.

MilhouseVH · 2017-10-27T16:58:56Z

Adding ?w=1 makes some of these commits easier to review. :)

MilhouseVH · 2017-10-27T21:50:20Z

scripts/build

+printf "%${BUILD_INDENT}c $(print_color CLR_BUILD "BUILD")    $PACKAGE_NAME $(print_color CLR_TARGET "($TARGET)")\n" ' '>&$SILENT_OUT
+export BUILD_INDENT=$((${BUILD_INDENT:-1}+$BUILD_INDENT_SIZE))
+
+# virtual packages dont must be build, they only contains dependencies, so dont go further here


This (old) comment is contradictory, maybe we can improve it?

# virtual packages don't build - they only contain dependencies, so don't go further here

?

MilhouseVH · 2017-10-27T21:53:06Z

How do you feel about replacing md5sum with sha256sum when calculating the stamps? Is it even worth doing (what are the chances of a collision)? Could be left for a separate PR.

InuSasha · 2017-10-27T22:02:36Z

sha256 takes more time. The advantage of sha256 over md5 is the security (but not relevant here).
I have never heard about problems with collisions.
I would leave it as is.

MilhouseVH · 2017-10-27T22:04:58Z

Fair enough.

InuSasha · 2017-10-28T08:56:34Z

clean builds of many archs run over night. I think, i had fixed every package, now.

MilhouseVH · 2017-10-29T14:48:04Z

Just to add some background on the buildsystem: remove grep from PKG_ARCH check change:

The following test cases now work:

TARGET_ARCH	PKG_ARCH	RESULT
arm		continue
arm	arm	continue
arm	any	continue
arm	any !x86_64	continue
x86_64	any !x86_64	exit 0
arm	aarch64 x86_64	exit 0
aarch64	aarch64 x86_64	continue
arm	!x86_64	exit 0
arm	!arm	exit 0
arm	!arm arm	exit 0
arm	!arm any	exit 0

The old grep based method had three issues:

it could not support PKG_ARCH="any -arm" (it used - to denote exclusion, which I've changed to ! as we don't currently have any packages using this notation)
it ran the risk of incorrectly matching an arch if $TARGET_ARCH is ever a subset of a longer arch, eg. TARGET_ARCH=arm would incorrectly match against PKG_ARCH="aarch64 arm64" (fictitious example but you get the point)
it uses sub-processes (which are evil 😄 )

This update fixes all three issues.

InuSasha · 2017-10-30T21:52:55Z

rebase and fix after merge of #2100

InuSasha · 2017-11-02T19:35:40Z

rebase and fix addon docker after #1670

lrusak · 2017-11-14T03:06:33Z

Generally I am ok with this. It doesn't really change anything but adds a bit more readability.

InuSasha · 2017-11-14T09:20:58Z

good to hear :)
i will do a rebase, this evening (UTC+1).

InuSasha · 2017-11-20T14:49:09Z

rebased on master.
include same changes need of merges on master

a generic with addons is done. build on the other project are running, actual.

InuSasha · 2017-11-21T10:24:10Z

other builds are done. looks good.
@lrusak this can go in, now

lrusak · 2017-11-21T19:01:58Z

@MilhouseVH can we hold all other PR's and test this please. If it builds just shove it in

I'd also like this to be squashed

MilhouseVH · 2017-11-21T20:04:16Z

OK I'll build it clean overnight and merge if OK (and squashed).

More efficient, slightly more functional. Can be a space delimited list of architectures. Architectures to be excluded can be specified with !ARCH. Allows "any !arm" to be interpreted as "any arch, but not arm". Blank/undefined is equivalent to "any".

CvH added BUILDSYSTEM IMPROVEMENT LE 9.0 WORK IN PROGRESS labels Oct 27, 2017

MilhouseVH reviewed Oct 27, 2017

View reviewed changes

MilhouseVH suggested changes Oct 27, 2017

View reviewed changes

MilhouseVH previously approved these changes Oct 27, 2017

View reviewed changes

MilhouseVH reviewed Oct 27, 2017

View reviewed changes

InuSasha force-pushed the feature/rewrite-build branch from d42527d to 6cc498c Compare October 30, 2017 21:52

InuSasha force-pushed the feature/rewrite-build branch from 6cc498c to b660b6e Compare November 2, 2017 19:34

InuSasha force-pushed the feature/rewrite-build branch from b660b6e to 6fbabf9 Compare November 20, 2017 14:42

CvH removed the WORK IN PROGRESS label Nov 21, 2017

InuSasha added 9 commits November 22, 2017 00:41

buildsystem: replace PKG_AUTORECONF

3846ec7

buildsystem: replace PKG_USE_NINJA

2bd83f8

buildsystem: replace PKG_USE_MESON

8c7dc83

buildsystem: replace PKG_USE_CMAKE

c90b78d

buildsystem: fix doubled PKG_TOOLCHAIN

2343a64

buildsystem: set PKG_TOOLCHAIN for packages, that need it

da38cd9

buildsystem: rename TOOLCHAIN other to manual

3b9fcfc

buildsystem: rework build script

3cd7be6

buildsystem: reduce cascaded if-blocks (stamp and virtual)

68ea042

InuSasha force-pushed the feature/rewrite-build branch from 6fbabf9 to 7f1800f Compare November 21, 2017 23:48

InuSasha and others added 2 commits November 22, 2017 00:49

buildsystem: set PKG_TOOLCHAIN for multiple packages

430f6e3

InuSasha force-pushed the feature/rewrite-build branch from 7f1800f to e0b833c Compare November 21, 2017 23:49

MilhouseVH approved these changes Nov 22, 2017

View reviewed changes

MilhouseVH merged commit 3192ddf into LibreELEC:master Nov 22, 2017

CvH mentioned this pull request Nov 25, 2017

addon compiling fixes #2258

Merged

InuSasha deleted the feature/rewrite-build branch November 26, 2017 11:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buildsystem: rework build script #2151

buildsystem: rework build script #2151

InuSasha commented Oct 27, 2017 •

edited

Loading

MilhouseVH Oct 27, 2017

InuSasha Oct 27, 2017

MilhouseVH Oct 27, 2017

MilhouseVH Oct 27, 2017

MilhouseVH commented Oct 27, 2017

InuSasha commented Oct 27, 2017

MilhouseVH Oct 27, 2017

InuSasha Oct 27, 2017

MilhouseVH commented Oct 27, 2017

MilhouseVH Oct 27, 2017

MilhouseVH commented Oct 27, 2017

InuSasha commented Oct 27, 2017

MilhouseVH commented Oct 27, 2017

InuSasha commented Oct 28, 2017

MilhouseVH commented Oct 29, 2017

InuSasha commented Oct 30, 2017

InuSasha commented Nov 2, 2017

lrusak commented Nov 14, 2017

InuSasha commented Nov 14, 2017

InuSasha commented Nov 20, 2017

InuSasha commented Nov 21, 2017

lrusak commented Nov 21, 2017 •

edited

Loading

MilhouseVH commented Nov 21, 2017

buildsystem: rework build script #2151

buildsystem: rework build script #2151

Conversation

InuSasha commented Oct 27, 2017 • edited Loading

MilhouseVH Oct 27, 2017

Choose a reason for hiding this comment

InuSasha Oct 27, 2017

Choose a reason for hiding this comment

MilhouseVH Oct 27, 2017

Choose a reason for hiding this comment

MilhouseVH Oct 27, 2017

Choose a reason for hiding this comment

MilhouseVH commented Oct 27, 2017

InuSasha commented Oct 27, 2017

MilhouseVH Oct 27, 2017

Choose a reason for hiding this comment

InuSasha Oct 27, 2017

Choose a reason for hiding this comment

MilhouseVH commented Oct 27, 2017

MilhouseVH Oct 27, 2017

Choose a reason for hiding this comment

MilhouseVH commented Oct 27, 2017

InuSasha commented Oct 27, 2017

MilhouseVH commented Oct 27, 2017

InuSasha commented Oct 28, 2017

MilhouseVH commented Oct 29, 2017

InuSasha commented Oct 30, 2017

InuSasha commented Nov 2, 2017

lrusak commented Nov 14, 2017

InuSasha commented Nov 14, 2017

InuSasha commented Nov 20, 2017

InuSasha commented Nov 21, 2017

lrusak commented Nov 21, 2017 • edited Loading

MilhouseVH commented Nov 21, 2017

InuSasha commented Oct 27, 2017 •

edited

Loading

lrusak commented Nov 21, 2017 •

edited

Loading