Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buildsystem: source functions earlier, validate project/arch earlier, refactor show_config #2323

Merged
merged 9 commits into from
Dec 31, 2017
Merged

buildsystem: source functions earlier, validate project/arch earlier, refactor show_config #2323

merged 9 commits into from
Dec 31, 2017

Conversation

MilhouseVH
Copy link
Contributor

@MilhouseVH MilhouseVH commented Dec 14, 2017

config/functions is not sourced until config/path, which means that none of the helper functions are available to the various options scripts (eg. $DISTRO/options) that are processed much earlier.

In addition, check_path and check_config are called after a package is sourced in config/path, which is a problem when using an invalid project/arch/device configuration, and sourcing a package (such as kodi) which includes nested references to other packages, eg.

-DLIBDVDCSS_URL=$ROOT/$SOURCES/libdvdcss/libdvdcss-$(get_pkg_version libdvdcss).tar.gz"

If you were to run:

( PROJECT=Generic ARCH=arm . config/options kodi )

then it will fail silently, as $(get_pkg_version libdvdcss) calls config/options libdvdcss which correctly outputs the ARCH config error (to stdout, which is processed as input in kodi/package.mk, so not visible to the user) and exit 1 brings the run to an end, with the user none the wiser.

Now, it fails as it should, before sourcing any packages, and outputs the error to stderr.

I'm not really sure if calling check_path after the package achieves anything more than what this PR now does, as the only way check_path could fail is if the package.mk performs cd /usr while being sourced, which seems a little unlikely.

I've also moved show_config() to a separate file as it's a large function that is being sourced multiple times but is only ever executed once.

@MilhouseVH
Copy link
Contributor Author

MilhouseVH commented Dec 17, 2017

Commit: buildsystem: get_module_dir() is expensive, optimize

Calling $(get_module_dir) is expensive, with the following call chain:

  1. Fork for $(get_module_dir)
  2. Fork for $(ls ...)
  3. Fork for $(get_build_dir linux)
  4. Fork for $(get_pkg_variable "$1" PKG_NAME)
    4.1 Sourcing config/options linux, with potentially more forks
  5. Fork for $(get_pkg_version "$1")
  6. Fork for $(get_pkg_variable "$1" PKG_VERSION)
    6.1 Sourcing config/options linux, with potentially more forks

$(get_module_dir) is normally called from $(get_full_module_dir), which is itself a 7th fork, and is usually called at least twice per package, and sometimes more often (eg. once per file, for multiple files).

This change will reduce the number of forks to 1 for $(get_module_dir) and a total of 2 forks for $(get_full_module_dir). The linux package will be sourced only twice, instead of twice for every invocation of $(get_module_dir).

Ping @HiassofT.

@MilhouseVH
Copy link
Contributor Author

Commit: buildsystem: set PKG_NAME and default PKG_*DESC only when sourcing a package

While testing the get_module_dir() change I realised that running:

DEBUG=no PROJECT=Generic DEVICE= ARCH=x86_64 CROSS_COMPILE= make -j8 release

results in:

./scripts/image release

such that config/options release is sourced.

Firstly, there is no package called release (an unknown package does not produce an error), however PKG_NAME, PKG_SHORTDESC and PKG_LONGDESC are being set as though the package is being found, even though no package is being sourced. Now, these variables will only be set once a package is actually sourced.

Secondly, if a release package (or mkimage, amlpkg or noobs package, which are all other possible arguments to scripts/image) is ever added, the build system might start to behave erratically as the release (etc.) package would then be unexpectedly sourced by scripts/image.

This change removes the $1 parameter that is passed to . config/options by scripts/image (and also scripts/checkdeps, which does not process any package arguments at all).

Removing $1 from scripts/image does have once consequence, which is that the package cache is no longer populated (and exported), so in scripts/image I have replaced $1 with a bogus "package" name simply to trigger the population of the package cache. It's not exactly ideal, but better than passing random arguments as package name that could one day become a real package.

@HiassofT
Copy link
Member

caching the module directory should be fine, but IMO it would be better to name the environment variable _CACHED_KERNEL_MODULE_DIR (similar to the package cache in config/path).

KERNEL_MODULE_DIR looks like some variable a builder might change (and he could when calling scripts/build) but that would make no sense of course.

@MilhouseVH
Copy link
Contributor Author

but IMO it would be better to name the environment variable _CACHED_KERNEL_MODULE_DIR (similar to the package cache in config/path).

Thanks - I've pushed this change.

@MilhouseVH
Copy link
Contributor Author

Commit: buildsystem: add configure_package function to finalise package initialisation

See comments: b3fe491#commitcomment-26335296

@MilhouseVH
Copy link
Contributor Author

MilhouseVH commented Dec 21, 2017

Initialising the package cache every time doesn't really impose that much overhead, so populate it every time config/options is invoked.

I've also fixed some weird indentation in the "package cache" commit, so use ?w=1 to review.

@MilhouseVH
Copy link
Contributor Author

MilhouseVH commented Dec 28, 2017

ping @HiassofT

Commit: config/functions: get_kernel_overlay_dir() returns a relative path, not absolute

The function get_kernel_overlay_dir() returns a value that is exclusively used as though it were a relative path, when in fact it is returning an absolute path.

For example:

ln -sf /storage/.config/firmware/ $INSTALL/$(get_full_firmware_dir)/updates

This results in the path $INSTALL//usr/lib/kernel-overlays/base/lib/firmware/updates (note the double //)

ln -sf /$(get_full_firmware_dir)/ $INSTALL/etc/firmware

This results in:

LibreELEC (community): devel-20171228015357-#1227-gbf7eef4 (RPi2.arm)
rpi22:~ # ls -la /etc/firmware
lrwxrwxrwx    1 root     root            44 Dec 28 01:56 /etc/firmware -> //usr/lib/kernel-overlays/base/lib/firmware/

Again, there is an issue with double //.

Changing the base function get_kernel_overlay_dir() to return a relative path ensures all existing code works as intended.

@HiassofT
Copy link
Member

good catch, looks fine to me!

@lrusak lrusak merged commit 7df2673 into LibreELEC:master Dec 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants