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: avoid if/then/else boiler plate when accessing hierarchy [RFC] #2432

Merged
merged 11 commits into from
Feb 3, 2018
Merged

buildsystem: avoid if/then/else boiler plate when accessing hierarchy [RFC] #2432

merged 11 commits into from
Feb 3, 2018

Conversation

MilhouseVH
Copy link
Contributor

This is something that's been bugging me for a while.

Currently we use this pattern which is repeated (with minor changes) in several places:

  if [ -n "$DEVICE" -a -f $PROJECT_DIR/$PROJECT/devices/$DEVICE/config/blah.conf ]; then
    cp $PROJECT_DIR/$PROJECT/devices/$DEVICE/config/blah.conf $INSTALL/etc
  elif [ -f $PROJECT_DIR/$PROJECT/config/blah.conf ]; then
    cp $PROJECT_DIR/$PROJECT/config/blah.conf $INSTALL/etc
  elif [ -f $DISTRO_DIR/$DISTRO/config/blah.conf ]; then
    cp $DISTRO_DIR/$DISTRO/config/blah.conf $INSTALL/etc
  else
    cp $PKG_DIR/config/blah.conf $INSTALL/etc
  fi

I think it would be better to use a helper function that takes care of locating the file or directory within the standard repo hierarchy (ie. project/device -> project -> distribution -> pkg etc.) so that the package doesn't have to care where the file is or might be, and packages can care less about projects, projects/devices, distros etc.

This change will eliminate almost all of this repeated code, while ensuring consistency whenever dealing with files and directories within the "standard repo hierarchy".

In terms of consistency, take this example in buysbox:

if [ -f $PROJECT_DIR/$PROJECT/busybox/busybox-target.conf ]; then
  BUSYBOX_CFG_FILE_TARGET=$PROJECT_DIR/$PROJECT/busybox/busybox-target.conf
else
  BUSYBOX_CFG_FILE_TARGET=$PKG_DIR/config/busybox-target.conf
fi

The project-specific config file is not being searched for in $PROJECT_DIR/$PROJECT/packages/busybox/config which I would argue is a more appropriate location than $PROJECT_DIR/$PROJECT/busybox.

With this change we will use the same hierarchy in $PROJECT_DIR (and $PROJECT/devices/$DEVICE and $DISTRO_DIR/$DISTRO) that is used in $PKG_DIR, which should help avoid confusion and errors.

In the busybox case, the config file (if ever created) could be located in:

$PROJECT_DIR/$PROJECT/packages/busybox/config/busybox-target.conf

which is analogous to the package directory location:

$PKG_DIR/config/busybox-target.conf

installer is updated in a similar way to busybox.

Another slight change is to plymouth-lite which now installs only splash-*.png images - image using any other name are ignored. By changing the logic for plymouth-lite it's also now possible to install a project specific config file, while continuing to use the standard distribution splash images.

samba will now install samba.conf.sample regardless of where smb.conf is found (see comment).

Some packages (eg. xorg-server, u-boot, busybox, etc.) were only searching in two or three locations (eg. $PROJECT and $PKG_DIR) but now they will be searching in all of the "standard" locations (including $DISTRO), which may have unexepected consequences if a config file is ever added in a location that wasn't being searched prior to this change. I don't think this is a problem - it just means we need to be more careful/aware of where we put files.

I've implemented the function in a slightly unusual way, in that it sets a global variable with the found path rather than echoing the found path (if no path is found the variable is left unset). This is to avoid using a temporary variable at the call site, or calling the function twice (once to determine if the path exists with -n "$(find_file_path ...)", and then again to process the path - each time forking a new process).

I've added optional logging whenever find_path is executed. I've made it optional as junk echoed while sourcing a package may be unexpected by some scripts, eg. repo-tool. Enable logging with VERBOSE_FIND_PATH=yes on the command line (or in $HOME/.libreelec/options).

@chewitt
Copy link
Member

chewitt commented Feb 1, 2018

I plan to merge this in 24h unless someone has an opinion against the change? Thanks

@MilhouseVH
Copy link
Contributor Author

I assume we're all good?

Copy link
Member

@chewitt chewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a nice simplification :)

@chewitt chewitt merged commit 0d13de3 into LibreELEC:master Feb 3, 2018
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.

2 participants