Re: [gentoo-dev] Bazel Build eclass

[Michał Górny]

On Sun, 2018-11-18 at 03:37 +0800, Jason Zaman wrote:
> Hey all,
>
> I've been using Bazel (https://bazel.build/) to build TensorFlow for a
> while now. Here is a bazel.eclass I'd like to commit to make it easier
> for packages that use it to build. It's basically bits that I've
> refactored out of the TensorFlow ebuild that would be useful to other
> packages as well. I have a bump to sci-libs/tensorflow-1.12.0 prepared
> that uses this eclass and have tested a full install.
>
> -- Jason
>
> # Copyright 1999-2018 Jason Zaman
> # Distributed under the terms of the GNU General Public License v2
>
> # @ECLASS: bazel.eclass
> # @MAINTAINER:
> # Jason Zaman <perfinion@g.o>
> # @AUTHOR:
> # Jason Zaman <perfinion@g.o>
> # @BLURB: Utility functions for packages using Bazel Build
> # @DESCRIPTION:
> # A utility eclass providing functions to run the Bazel Build system.
> #
> # This eclass does not export any phase functions.
>
> case "${EAPI:-0}" in
> 	0|1|2|3|4|5|6)
> 		die "Unsupported EAPI=${EAPI:-0} (too old) for ${ECLASS}"
> 		;;
> 	7)
> 		;;
> 	*)
> 		die "Unsupported EAPI=${EAPI} (unknown) for ${ECLASS}"
> 		;;
> esac
>
> if [[ ! ${_BAZEL_ECLASS} ]]; then
>
> inherit multiprocessing toolchain-funcs
>
> BDEPEND=">=dev-util/bazel-0.19"
>
> # @FUNCTION: bazel_get_flags
> # @DESCRIPTION:
> # Obtain and print the bazel flags for target and host *FLAGS.
> #
> # To add more flags to this, append the flags to the
> # appropriate variable before calling this function
> bazel_get_flags() {
> 	local i fs=()
> 	for i in ${CFLAGS}; do
> 		fs+=( "--conlyopt=${i}" )
> 	done
> 	for i in ${BUILD_CFLAGS}; do
> 		fs+=( "--host_conlyopt=${i}" )
> 	done
> 	for i in ${CXXFLAGS}; do
> 		fs+=( "--cxxopt=${i}" )
> 	done
> 	for i in ${BUILD_CXXFLAGS}; do
> 		fs+=( "--host_cxxopt=${i}" )
> 	done
> 	for i in ${CPPFLAGS}; do
> 		fs+=( "--conlyopt=${i}" "--cxxopt=${i}" )
> 	done
> 	for i in ${BUILD_CPPFLAGS}; do
> 		fs+=( "--host_conlyopt=${i}" "--host_cxxopt=${i}" )
> 	done
> 	for i in ${LDFLAGS}; do
> 		fs+=( "--linkopt=${i}" )
> 	done
> 	for i in ${BUILD_LDFLAGS}; do
> 		fs+=( "--host_linkopt=${i}" )
> 	done
> 	echo "${fs[*]}"
> }
>
> # @FUNCTION: bazel_setup_bazelrc
> # @DESCRIPTION:
> # Creates the bazelrc with common options that will be passed
> # to bazel. This will be called by ebazel automatically so
> # does not need to be called from the ebuild.
> bazel_setup_bazelrc() {
> 	if [[ -f "${T}/bazelrc" ]]; then
> 		return
> 	fi
>
> 	# F: fopen_wr
> 	# P: /proc/self/setgroups
> 	# Even with standalone enabled, the Bazel sandbox binary is run for feature test:
> 	# https://github.com/bazelbuild/bazel/blob/7b091c1397a82258e26ab5336df6c8dae1d97384/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java#L61
> 	# https://github.com/bazelbuild/bazel/blob/76555482873ffcf1d32fb40106f89231b37f850a/src/main/tools/linux-sandbox-pid1.cc#L113
> 	addpredict /proc
>
> 	mkdir -p "${T}/bazel-cache" || die
> 	mkdir -p "${T}/bazel-distdir" || die
>
> 	cat > "${T}/bazelrc" <<-EOF || die
> 	startup --batch

Maybe indent this stuff to make it stand out from ebuild code.

>
> 	# dont strip HOME, portage sets a temp per-package dir
> 	build --action_env HOME
>
> 	# make bazel respect MAKEOPTS
> 	build --jobs=$(makeopts_jobs)
> 	build --compilation_mode=opt --host_compilation_mode=opt
>
> 	# FLAGS
> 	build $(bazel_get_flags)
>
> 	# Use standalone strategy to deactivate the bazel sandbox, since it
> 	# conflicts with FEATURES=sandbox.
> 	build --spawn_strategy=standalone --genrule_strategy=standalone
> 	test --spawn_strategy=standalone --genrule_strategy=standalone
>
> 	build --strip=never
> 	build --verbose_failures --noshow_loading_progress
> 	test --verbose_test_summary --verbose_failures --noshow_loading_progress
>
> 	# make bazel only fetch distfiles from the cache
> 	fetch --repository_cache="${T}/bazel-cache/" --distdir="${T}/bazel-distdir/"
> 	build --repository_cache="${T}/bazel-cache/" --distdir="${T}/bazel-distdir/"
>
> 	build --define=PREFIX=${EPREFIX%/}/usr
> 	build --define=LIBDIR=\$(PREFIX)/$(get_libdir)
>
> 	EOF
>
> 	tc-is-cross-compiler || \
> 		echo "build --nodistinct_host_configuration" >> "${T}/bazelrc" || die

Don't do || chains, they are unreadable.

> }
>
> # @FUNCTION: ebazel
> # @USAGE: [<args>...]
> # @DESCRIPTION:
> # Run bazel with the bazelrc and output_base.
> #
> # If $MULTIBUILD_VARIANT is set, this will make an output_base
> # specific to that variant.
> # bazel_setup_bazelrc will be called and the created bazelrc
> # will be passed to bazel.
> #
> # Will automatically die if bazel does not exit cleanly.
> ebazel() {
> 	bazel_setup_bazelrc
>
> 	# Use different build folders for each multibuild variant.
> 	local base_suffix="${MULTIBUILD_VARIANT+-}${MULTIBUILD_VARIANT}"

Any reason not to use BUILD_DIR instead of reinventing it?

> 	local output_base="${WORKDIR}/bazel-base${base_suffix}"
> 	mkdir -p "${output_base}" || die
>
> 	einfo Running: bazel --output_base="${output_base}" "$@"
> 	bazel --bazelrc="${T}/bazelrc" --output_base="${output_base}" $@ || die "ebazel $@"

The common practice is to echo >&2 it.  Also, you output different
arguments than you execute which is going to confuse the hell out of
users who'll end up having to debug this.  You can use a trick like
the following to avoid typing args twice:

  set -- bazel --bazelrc...
  echo "${*}" >&2
  "${@}" || die ...

> }
>
> # @FUNCTION: bazel_load_distfiles
> # @USAGE: <distfiles>...
> # @DESCRIPTION:
> # Populate the bazel distdir to fetch from since it cannot use
> # the network. Bazel looks in distdir but will only look for the
> # original filename, not the possibly renamed one that portage
> # downloaded. If the line has -> we to rename it back. This also
> # handles use-conditionals that SRC_URI does.

Why oh why do you have to implement custom parser for the ebuild syntax?
 That's just asking for horrible failures.

> #
> # Example:
> # @CODE
> # bazel_external_uris="http://a/file-2.0.tgz
> #     python? ( http://b/1.0.tgz -> foo-1.0.tgz )"
> # SRC_URI="http://c/${PV}.tgz
> #     ${bazel_external_uris}"
> #
> # src_unpack() {
> #     unpack ${PV}.tgz
> #     bazel_load_distfiles "${bazel_external_uris}"
> # }
> # @CODE
> bazel_load_distfiles() {
> 	local src dst uri rename
>
> 	[[ "$@" ]] || die "Missing args"
> 	mkdir -p "${T}/bazel-distdir" || die
>
> 	while read uri rename dst; do
> 		src="${uri##*/}"
> 		[[ -z $src ]] && continue

Please use ${foo} syntax in ebuilds, consistently.

> 		if [[ "$rename" != "->" ]]; then
> 			dst="${src}"
> 		fi
>
> 		[[ ${A} =~ ${dst} ]] || continue

Why are you doing regex match here?  Last I checked, we didn't use
regular expressions in SRC_URI.

>
> 		if [[ "$dst" == "$src" ]]; then
> 			einfo "Copying $dst to bazel distdir ..."
> 		else
> 			einfo "Copying $dst to bazel distdir $src ..."

Are you using src and dst to mean the opposite?

> 		fi
> 		dst="$(readlink -f "${DISTDIR}/${dst}")"

Looks like you are hardcoding hacks for implementation details which
indicates whatever you're doing is a very bad idea, and is going to fail
whenever the implementation is subtly different than what you've worked
around so far.

> 		ln -s "${dst}" "${T}/bazel-distdir/${src}" || die
> 	done <<< "$(sed -re 's/!?[A-Za-z]+\?\s+\(\s*//g; s/\s+\)//g' <<< "$@")"

Please don't use horribly unreadable sed expressions.  This just means
that whoever will have to touch this eclass in the future will wish you
were never recruited.

> }
>
> _BAZEL_ECLASS=1
> fi
>
>
>

-- 
Best regards,
Michał Górny