Conversation

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.

This comment has been minimized.

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.

This comment has been minimized.

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.

This comment has been minimized.

edited

@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.

This comment has been minimized.

edited

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.

This comment has been minimized.

edited

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:

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):

This comment has been minimized.

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

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.