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: unset all PKG_* variables before sourcing a new package #2072

Merged
merged 1 commit into from Oct 7, 2017

Conversation

@MilhouseVH
Copy link
Contributor

commented Oct 7, 2017

This follows on from a discussion on Slack arising out of #2067, which is adding a new package variable PKG_ADDON_BROKEN.

Adding new variables in a package leads to the risk of those variables leaking over into a subsequent package unless they are unset before the new package is sourced. To prevent leakage we currently maintain a list of variables that must be unset in config/path, which is almost always going to be missing one or more variables.

This PR attempts to address this issue by automatically unsetting ALL PKG_* variables before sourcing a new package.

I've tested this with clean builds of RPi, RPi2 and Generic, and no problems.

This change does add some overhead - about 10 seconds extra to a Generic build on AMD FX-8350 which isn't excessive in the scheme of things - but does mean we don't have to worry about critical package variables being accidentally leaked and causing unpredictable behaviour.

Potentially all variables set within a package could/should include the PKG_ prefix, for example where TVH_TRANSCODING becomes PKG_TVH_TRANSCODING. however the decision really is whether the variable being set is ever likely to be used by another package - if it is then it should definitely use the PKG_ prefix, and if not then... meh.

@CvH

This comment has been minimized.

Copy link
Member

commented Oct 7, 2017

As it doesn't hurt to have some standard for variable naming at package.mk and of the benefit of one less thing to care about this is good to go I think.
The deeper LE building system shenanigans are out of my scope to really review it.

@vpeter4

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2017

I think for loop in reset_pkg_vars function could be optimized by grepping all package.mk files and extracting PKG_ variables only once? On first grep one flag variable would be set to prevent grepping again.
Not sure how faster would be but it should be considerable regarding that awk is called on every package.

@CvH
CvH approved these changes Oct 7, 2017
@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2017

@vpeter4 grepping all packages sounds far more complicated/error prone, and also slower - there are over 770 packages that would need to be grepped/parsed (at least half of which wouldn't even be needed for any particular build - a Generic build uses only about 360 packages, a create_addon build might only need one package).

Also, this current implementation unsets only those PKG_* variables that are actually set, which might be none or only a handful of variables - if we are unsetting all variables parsed from all package.mk's then we'd be unsetting dozens more variables than is necessary before sourcing each package.

@MilhouseVH MilhouseVH force-pushed the MilhouseVH:le90_unset_pkg_vars branch from 38e1596 to 32560f0 Oct 7, 2017

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2017

I've pushed a change which simplifies the awk processing:

-  for var in $(set | awk '/^PKG_.*=/ { gsub(/=.*$/, ""); print }'); do
+  for var in $(set | awk -F= '/^PKG_.*=/ { print $1 }'); do
@vpeter4

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2017

Well, I don't know what is faster but 10 seconds is a lot. That's why I decided to see how much does it take. But I'm not sure how you even got 10 sec. For me making tar is only few seconds difference (Odroid_C2 project). Which is fine. For Generic it would take few more.

And just for reference

time find packages projects -type f -name package.mk -exec cat {} \; | awk '/^PKG_.*=/ { gsub(/=.*$/, ""); print }' | sort -u
real    0m0.377s
user    0m0.044s
sys     0m0.080s

time for i in $(seq 1 55); do echo "PKG_SOMETHING_$i" | while read var; do unset -v $var; done; done
real    0m0.064s
user    0m0.000s
sys     0m0.020s

Better having code nicer :)

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2017

Well, I don't know what is faster but 10 seconds is a lot.

That is 10 seconds added to a build that might take up to 4 hours.

The 10 seconds I calculated by performing PROJECT=Generic ARCH=x86_64 make release, and once I had a build I repeated this several times with the new and old unset methods. As the subsequent builds had no package changes the builds were just iterating over the 309 packages needed by the build (calling scripts/build > scripts/unpack > scripts/install for each package) before producing a new tar. Based on 3 runs each the new method is 10 seconds slower in total per build, which is 0.032s extra to process each package (which is not to say that reset_pkg_vars takes 0.032 seconds to execute, as processing one package will result in multiple - at least 3 - calls to reset_pkg_vars).

The problem with parsing package.mk is that you also need to take into consideration those PKG_* variables that aren't at the start of the line, for example when being set conditionally:

[ "$TARGET_ARCH" = "x86_64" ] && PKG_SOMETHING="blah"

or that might be indented:

if [ "$TARGET_ARCH" = "x86_64" ]; then
  PKG_SOMETHING="blah"
fi

which is why parsing package.mk is inherently more complex.

@vpeter4

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2017

Agree, issues can escalates quickly out of control with my approach. And when considered all corner cases the question would be if final result would be much better. Probably not that much.

reset_pkg_vars() {
local vars var

for var in $(set | awk -F= '/^PKG_.*=/ { print $1 }'); do

This comment has been minimized.

Copy link
@mglae

mglae Oct 7, 2017

Contributor

bash allows for var in ${!PKG_*}; do

This comment has been minimized.

Copy link
@MilhouseVH

MilhouseVH Oct 7, 2017

Author Contributor

Oh... very nice, thanks! :)

Is this likely to be compatible with all versions of bash?

This comment has been minimized.

Copy link
@MilhouseVH

MilhouseVH Oct 7, 2017

Author Contributor

I've added an extra commit using your solution which I will squash if there are not likely to be any compatibility issues.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2017

Some stats, using the latest ${!PKG_*} method suggested by @mglae.

I'm using this test script, which can be run in the root of the LE repo:

#!/bin/bash

MAXPKG=${1:-0}
LOOP=${2:-3}

PROFILE=Generic
ARCH=x86_64

getpkgs() {
  local path
  while read -r path; do
    basename $(dirname ${path})
  done <<< "$(find packages -not -path "packages/addons/*" -name package.mk)" | sort
}

exit() {
  :
}

dotest() {
  local c s pkg
  c=0
  s=$(date +%s)
  for pkg in ${PKGLIST}; do
    c=$((c+1))
    [ ${MAXPKG} -ne 0 -a ${c} -ge ${MAXPKG} ] && break
    source config/options $pkg
    source config/options $pkg
    source config/options $pkg
  done

  echo "ELAPSED: $(($(date +%s) - s)) seconds, $c packages x3 (LOOP #${loop})"
}

PKGLIST="$(getpkgs)"
for loop in $(seq ${LOOP}); do
  dotest
done

It sources each package three times as this is a fairly representative test of what happens during a build (build > unpack > install). That said, the script does processes far more packages than any build, eg. Generic processes only 309 packages.

Timings are in seconds, for 557 packages, average of 3 runs (row #1 is the current "traditional" method):

Method Time Delta to #1 Delta/package Delta/source
1. PKG_BLAH="" 143 0 0.000 0.000
2. ${!PKG_*} 148 5 0.009 0.003
3. set | awk 165 22 0.039 0.013

Based on a Generic build with 309 packages, the new method #2 should add about 2.8 (309 * 0.009) seconds to the total build time.

@lrusak

This comment has been minimized.

Copy link
Member

commented Oct 7, 2017

This looks pretty good to me. The only thing I would maybe add is a default PKG_VERSION, this allows us to get rid of PKG_VERSION in packages that don't really have a version such as packages/sysutils/entropy

Otherwise I am happy to get this in.

@lrusak
lrusak approved these changes Oct 7, 2017
@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2017

What would you suggest as a default PKG_VERSION?

@lrusak

This comment has been minimized.

Copy link
Member

commented Oct 7, 2017

Doesn't really matter to me :)

packages/addons/service/bluetooth-audio/package.mk:PKG_VERSION="0"
packages/addons/service/slice/package.mk:PKG_VERSION="0"
packages/sysutils/entropy/package.mk:PKG_VERSION="0"
packages/mediacenter/JsonSchemaBuilder/package.mk:PKG_VERSION="0"
packages/mediacenter/TexturePacker/package.mk:PKG_VERSION="0"

@MilhouseVH MilhouseVH force-pushed the MilhouseVH:le90_unset_pkg_vars branch from 985323c to 24bfb0e Oct 7, 2017

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2017

I've changed PKG_VERSION to default to 0.0. Also squashed the commits.

@MilhouseVH MilhouseVH force-pushed the MilhouseVH:le90_unset_pkg_vars branch from 24bfb0e to 98baf44 Oct 7, 2017

@lrusak lrusak merged commit adf3f57 into LibreELEC:master Oct 7, 2017

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2017

Back to PKG_VERSION="0.0invalid" - changing this now might cause unwanted issues elsewhere (eg. distro-tool).

My guess is that 0.0invalid as a default is meant to avoid confusion with 0.0 which could potentially be a valid version.

We should change the default PKG_VERSION in a separate PR when cleaning up those packages that don't have/need a version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.