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

Update scripts/Build*.sh to build the libraries listed in the current README #5

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nkaminski
Copy link
Contributor

@nkaminski nkaminski commented Dec 3, 2024

This changeset updates the scripts/Build*.sh scripts to build the libraries listed in the current version of the readme as well as introduces the following minor changes / improvements.

  • MACOSX_DEPLOYMENT_TARGET defaults to 11.0 but can be overwritten if predefined in the environment
  • Automatic CPU core count detection - build will use all CPUs on the host by default
  • Automatic build host architecture detection using uname -p
  • Test for Homebrew as well as installation directories under /opt pre-run
  • xLights source directory defaults to ./xLights, but can be overridden by defining XL_DIR

This was largely driven by my intention to perform a local debug build of xLights on macOS in order to further debug xLightsSequencer/xLights#5015

…he current README

This changeset updates the scripts/BuildLibsRelease.sh script to build the
libraries listed in the current version of the readme as well as introduces
the following minor changes / improvements

* MACOSX_DEPLOYMENT_TARGET defaults to 11.0 but can be overwritten if predefined in the environment
* Automatic CPU core count detection - build will use all CPUs on the host by default
* Automatic build host architecture detection using uname -p
* Test for Homebrew as well as installation directories under /opt pre-run
* xLights source directory defaults to ./xLights, but can be overdidden by defining XL_DIR
@nkaminski nkaminski force-pushed the libs_build_script_update branch from 91ac62b to b3fbeb4 Compare December 3, 2024 05:06
@nkaminski nkaminski marked this pull request as ready for review December 3, 2024 05:06
@nkaminski nkaminski changed the title Update scripts/BuildLibsRelease.sh to build the libraries listed in the current README Update scripts/Build*.sh to build the libraries listed in the current README Dec 3, 2024
Copy link

@DanInProgress DanInProgress left a comment

Choose a reason for hiding this comment

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

Just a few suggestions, not sure if this is intended to be built on mac os or built elsewhere and then copied? probably good enough if it will only ever be run on mac os

scripts/BuildLibsRelease.sh Show resolved Hide resolved
sudo chgrp -R staff /opt/local*
sudo chmod -R g+w /opt/local*
# System dependency checks...
if [ ! -d /opt/local/lib ] || [ ! -d /opt/local/libdbg ] || [ ! -d /opt/local/bin ] || [ ! -d /opt/local/bin ] ; then

Choose a reason for hiding this comment

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

if you're restricting to bash, more reliable to use [[ instead of test

Copy link
Contributor Author

@nkaminski nkaminski Dec 3, 2024

Choose a reason for hiding this comment

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

Fair, can refactor.

sudo chmod -R g+w /opt/local*
# System dependency checks...
if [ ! -d /opt/local/lib ] || [ ! -d /opt/local/libdbg ] || [ ! -d /opt/local/bin ] || [ ! -d /opt/local/bin ] ; then
echo "/opt/local/bin, /opt/local/lib, /opt/local/libdbg and /opt/local/include must exist and be writable!"

Choose a reason for hiding this comment

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

mild recommend for using printf over echo

here are some sample helpers that will do what you might expect
https://gist.github.com/DanInProgress/e444b7815745da4c1117f78dc9ebb2d3

scripts/BuildLibsRelease.sh Show resolved Hide resolved
export CFLAGS="-g -O3 -flto=thin -Wall -fno-stack-protector -fno-common ${XL_TARGETS} ${OSX_VERSION_MIN}"
make -j4 all CFLAGS="$CFLAGS" MYLDFLAGS="$CFLAGS"
export CFLAGS="-g -O3 -flto=thin -Wall -fno-stack-protector -fno-common ${XL_TARGETS} ${OSX_VERSION_MIN} "
make -j $NUM_CPUS all CFLAGS="$CFLAGS" MYLDFLAGS="$CFLAGS"
Copy link

@DanInProgress DanInProgress Dec 3, 2024

Choose a reason for hiding this comment

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

with the large number of builds, it might be more legible debuggable to any of:

  • split this into multiple shells scripts that can succeed or fail individually
  • use make with per-target exported flags to update the dependent builds and track their success
  • shelling out will also help with env/workdir management and isolation

edit to add context from other comment

you can subshell out without a separate script using ( ... ), but beware changes in set -e behavior due to the compound command with either {...} or (...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be in favor of this however this would be a functional change from the steps in the readme. Would be interested as to other's thoughts here.

scripts/BuildLibsRelease.sh Show resolved Hide resolved
@nkaminski
Copy link
Contributor Author

Thank you @DanInProgress for the review - a second pair of eyes is always a favorable thing for less trivial changes.

With regards to the build host, this will always be built on a macOS machine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants