Openwrt 2410 branch package cleanup v2 #1028
Conversation
Signed-off-by: Andrew MacIntyre <[email protected]>
Where it makes sense the Gargoyle kconfig can be used to set symbol and package defaults for the directly supported targets to reduce reliance on, and maintenance of, the individual target profile config files. Enhancements include: - include some additional general config symbol settings for all builds - add additional menu items for package groups such as target specific ethernet drivers, optional ethernet drivers, optional wifi drivers and optional utilities (including opkg) - fine tune some symbol and/or package inclusions for specific targets Limitations of the kconfig system mean that symbols defaulting to enabled (i.e. "=m" or "=y") that need be disabled (i.e. set "=n") can't be incorporated in the Gargoyle kconfig and must be unset in the target profile config(s) instead. Signed-off-by: Andrew MacIntyre <[email protected]>
- kmod-usb-net is a direct dependency of all the specified drivers so doesn't need to be explicitly specified - add an extra USB network driver for the RPi5 Signed-off-by: Andrew MacIntyre <[email protected]>
The extra packages aren't direct dependencies but other parts of Gargoyle expect them to be available as part of the firewall functionality. Signed-off-by: Andrew MacIntyre <[email protected]>
The extra package isn't a direct dependency but other parts of Gargoyle expect it to be available as part of the firewall3 functionality. Signed-off-by: Andrew MacIntyre <[email protected]>
The PROVIDES=msmtp for the nossl variant of the msmtp package defeats
the dependency for the SSL variant of the plugin, resulting in only
the nossl variant of the msmtp package being built by default.
Fix this by deactivating the PROVIDES and adding explicit msmtp package
variant selection in both plugin variants to match the SSL variant.
NB: PROVIDES has been commented out rather than removed to ease
restoration in any future restructure of these packages.
Signed-off-by: Andrew MacIntyre <[email protected]>
Unfortunately it appears that having the build statements with the OpenSSL option first ensures that openvpn-gargoyle-openssl gets built as the default despite other settings that should have the mbedtls variant built, so reorder the statements with mbedtls variants first. Signed-off-by: Andrew MacIntyre <[email protected]>
…iable Making the default library selection depend on an an existing external (to the package) symbol improves default libary selection reliability. Additionally minor editorial improvements to the package Makefile to match common OpenWrt packaging practice. Signed-off-by: Andrew MacIntyre <[email protected]>
OpenWrt's default package machinery runs into trouble with the mvebu target, possibly provoked by other recent changes, and images fail to build because of a clash between wpad-basic-mbedtls (which gets added to an image first) and wpad-mbedtls. A general solution would be to scan all target image recipes in the OpenWrt build directory and substitute wpad-mbedtls for wpad-basic-mbedtls e.g. using sed. No other target currently experiences this problem so for now just implement a target specific patch for the substitution rather than try and change the build machinery to implement a general solution. Signed-off-by: Andrew MacIntyre <[email protected]>
Now that a `make defconfig` is done with each build, the target profile config file only needs the bare minimum config statements that result in the required buildable config - OpenWrt refers to such a config file to as a `diffconfig` file, which `make defconfig` turns into a buildable config file. This approach achieves much better maintainability for Gargoyle's supported devices by focussing only on the specific non-default settings required for the profile and each device, with additional devices only needing the relevant device statements to be added for their images to be built (the profile_images patterns also need to added). There are only a relatively small number of non-default settings required for Gargoyle builds and it is much easier to maintain this small number in isolation without having to wade through a few thousand lines found in a full config file. Going forward the intention would be to maintain these configs only by changing settings that can't be satisfactorily adjusted any other way. Preference should given to making setting adjustments through the global Gargoyle Config.in or package configurations where possible. However settings enabled by default in OpenWrt that Gargoyle needs disabled can frequently only be disabled by explicit profile config settings (e.g. default packages, and some kernel and BusyBox settings). There are a number of OpenSSL related packages that won't be built (e.g. wpad-openssl) with these configs; if these packages are still required to be built by default changes should be made to the global Gargoyle Config.in or gargoyle-extras package structure to implement this rather than including them in these configs. The custom default config has been refreshed from the ath79 default config. Signed-off-by: Andrew MacIntyre <[email protected]>
lantis1008
left a comment
lantis1008
left a comment
There was a problem hiding this comment.
Hi, please see inline minor commentary.
I'm slightly uncomfortable with the diffconfig change. On one hand I think it is an excellent simplification. On the other, I frequently look at the full configs to understand exactly what is in each image and have now lost that verbosity. I also can't easily compare changes between kernel updates or major version updates to understand why something might have gone wrong. Or upstream starts to suddenly ship a new default package which is quite large and my investigative workload goes up.
I'm 50:50 on this!
Thoughts?
|
There are benefits too. It does make other things (like busybox default algo) more obvious. |
I understand - everything's a compromise :-/ and there are definitely pros & cons. I came down in favour of the diffconfig approach on the basis of:
I don't think I articulated it well if at all, but it was the latter point above which really drove me to make this whole series of changes towards leveraging defaults and dependencies, as when I analysed the various profile configs I kept running into oddball packages that I couldn't see why they should be included. So while I initially looked at rationalising the configs with a script, I kept coming back to the fact that that did little to avoid the cruft creeping in again. All of the changes in this PR won't solve everything of course, but I believe they will go a fair way towards minimising overall profile maintenance. However as you say you also have to manage your cognitive load during troubleshooting and that is a valid non-trivial factor in the face of a significantly different view of a profile's config. I deliberately made the diffconfig change the last commit in the series for 2 reasons: 1) it naturally built on the prior commits and wrapped things up, and 2) if you decided not to proceed with it it isn't critical to the success of the prior commits which could be easily cherry picked out of the PR. The diffconfig change is also easy to revert, even just on a profile by profile basis if need be, and new targets don't have to start off with a diffconfig. The machinery that's in place allows using diffconfigs where it makes sense, and I think it makes a lot of sense for stable mature profiles; it may not make sense for new and/or problematic profiles. The commit in this PR is just a bulk conversion to start the ball rolling, based on analysis of the current configs and minimising discrepancies between pre- and post- change configs as used for actual builds. |
|
Your last point is a good sales pitch actually. in that light I think we can accept the change and rethink the workflow of doing major updates. |
|
Have you done a full compile test across all targets? I'm seeing this on ath79 (I have not checked further yet). I think that when you see these the configs are not able to self resolve and you can end up with nasty fallout. |
|
I did numerous builds while testing these changes, including an all targets build before I put up the revised PR. I investigated the ath10k related "recursive dependency" issues in particular as deeply as I could and my conclusion was that they're bogus as it comes down to limitations in the kconfig parser as there is in fact no recursion, although there are some complexities to the logic. In every build I did with these errors reported the correct config was generated. The So yes I agree caution is warranted when these sorts of errors are reported but on investigation I'm comfortable that these specific errors (i.e. the ones you've shown above) can be treated as false positives as correct .configs are produced for every profile in every target (I checked them all, multiple times!), and there weren't any image build failures I could identify related to dependencies. In hindsight I should have highlighted this behaviour and that I had investigated each example I encountered - mea culpa. |
|
I believe I've found a way to workaround the ath10k issue - the patch looks like this: i.e. changing the symbol prefix from That leaves the |
|
If you wanted to bump to OpenWrt 24.10.5 first I don't expect that to affect anything in this PR - the |
|
New commit to pickup the ath10k changes is ok. I’ve finished a 24.10.5 build overnight I need to verify it and I’ll publish. |
the addition of alternative ath10k related packages to the Gargoyle kconfig resulted in reports of recursive dependency errors when in fact there are none. Suppress these unhelpful errors by testing against MODULE_DEFAULT prefixed ath10k symbols instead of the PACKAGE prefixed ath10k symbols. The MODULE_DEFAULT prefixed symbols are crystalisations of the underlying package selection logic associated with ath10k packages specified as device packages in image recipes, and using them simplifies the logic complexity to be evaluated by the kconfig parser circumventing the reported errors. Signed-off-by: Andrew MacIntyre <[email protected]>
|
ath10k fixup commit added |
2 participants
A series of commits to simplify and streamline how Gargoyle target profile image configurations are managed with the goal to minimise maintenance for target profile configurations once they're created. The general approach is to leverage default setting mechanisms supported by the underlying OpenWrt build system as much as possible.
Changes compared to #1027:
procd-ujailpatch to remove it as a default package has been droppedopkgpatch to remove it as a default package is retained, the package is now selected inConfig-gargoyle.into be built but not included in imagesSupercedes #1027
Depends on #1026