[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gentoo-dev
Subject:    Re: [gentoo-dev] Bazel Build eclass
From:       Michał_Górny <mgorny () gentoo ! org>
Date:       2018-11-18 8:01:00
Message-ID: 1542528060.1293.2.camel () gentoo ! org
[Download RAW message or body]


On Sun, 2018-11-18 at 15:37 +0800, Jason Zaman wrote:
> On Sat, Nov 17, 2018 at 11:54:24PM +0100, Michał Górny wrote:
> > 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@gentoo.org>
> > > # @AUTHOR:
> > > # Jason Zaman <perfinion@gentoo.org>
> > > # @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/7b091c1397a82258e26ab5336df6c8 \
> > > dae1d97384/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.
> 
> ok
> 
> > > }
> > > 
> > > # @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?
> 
> Isnt $BUILD_DIR $S by default? output_base is a bazel thing with a weird
> structure and has nothing to do with the source tree. Doing it this way
> meant python_foreach_impl was easy. but I might be confused and will
> look into it again and see if it can be better. this way means with and
> without multibuild just works tho.

There's no default.  It's defined by the few eclasses such as multibuild
and cmake-utils.  You just need to default if it other eclasses don't
set 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 ...
> 
> Oh, forgot that trick. yeah I'll do that.
> 
> > > }
> > > 
> > > # @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.
> 
> Yeah ... so thats the problem with bazel and the main reason for the
> eclass. Bazel has this idea that downloading and unpacking random
> tarballs during build is okay. Now bazel has an option when it needs to
> fetch something it will look in the dir --distdir= points to instead and
> read the file instead of trying to fetch from the (sandboxed thus
> non-existent) internet. But bazel only knows the original filenames not
> the names that portage renamed things to so I need to rename them back
> to the original names.
> 
> The other option would be to have ebuilds maintain a second list of urls
> on top of the SRC_URI one which seems way more of a maintenance burden,
> especially when use-flags are involved. So all I really do here is strip
> the use? and ()'s and take the list of urls and put them in the
> bazel-distdir with the original filenames.
> 
> If you can think of a better way to accomplish that I'd love to hear it
> tho. I'll add more comments about what it does but bazel is weird and
> just utterly fails if it cant read the tarballs. And unpacking things
> myself doesnt work because of the weird file structure bazel uses so
> knowing where to put them is non-trivial.

What if the original URL goes down and somebody replaces 'foo -> bar'
with 'mirror://gentoo/bar'?

> 
> > > #
> > > # 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.
> 
> ok
> 
> > > 		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.
> 
> It was to skip this one if its not in $A (eg disabled with a use-flag)
> I'll change it to == *${dst}* instead then.
> 
> > > 
> > > 		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?
> 
> No, this is right. its SRC_URI="src -> dst" so I need to rename dst back
> so its named src.
> 
> > > 		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.
> 
> I can skip this line then no problem. It will just end up a link to a
> link but thats fine.
> 
> > > 		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.
> 
> I will split it up and add more comments. It just removes "use? (",
> "!use? (", and " )". If I don't remove the useflag bits first then figuring out
> if its a rename or not becomes a lot more complicated. If you have
> other ideas im all ears.

The syntax is token-oriented, so just tokenize it and use a state
machine to parse it if you must.

> 
> 
> > 
> > > }
> > > 
> > > _BAZEL_ECLASS=1
> > > fi
> > > 
> > > 
> > > 
> > 
> > -- 
> > Best regards,
> > Michał Górny
> 
> 
> 

-- 
Best regards,
Michał Górny


["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]