Re: [gentoo-dev] [PATCH 1/2] bzr.eclass: Reinstate eclass

[Ulrich Mueller]

>>>>> On Sat, 25 Sep 2021, Michał Górny wrote:

>> +EBZR="bzr.eclass"

> Why do we need this?  It seems as if someone is reinventing ${ECLASS}.

Replaced by ${ECLASS}.

>> +# @ECLASS-VARIABLE: EBZR_STORE_DIR

> @USER_VARIABLE?

Added.

>> +# @DESCRIPTION:
>> +# The directory to store all fetched Bazaar live sources.
>> +: ${EBZR_STORE_DIR:=${PORTAGE_ACTUAL_DISTDIR:-${DISTDIR}}/bzr-src}
>> +
>> +# @ECLASS-VARIABLE: EBZR_UNPACK_DIR
>> +# @DESCRIPTION:
>> +# The working directory where the sources are copied to.
>> +: ${EBZR_UNPACK_DIR:=${WORKDIR}/${P}}
>> +
>> +# @ECLASS-VARIABLE: EBZR_INIT_REPO_CMD
>> +# @DESCRIPTION:
>> +# The Bazaar command to initialise a shared repository.
>> +: ${EBZR_INIT_REPO_CMD:="bzr init-repository --no-trees"}
>> +
>> +# @ECLASS-VARIABLE: EBZR_FETCH_CMD
>> +# @DESCRIPTION:
>> +# The Bazaar command to fetch the sources.
>> +: ${EBZR_FETCH_CMD:="bzr branch --no-tree"}
>> +
>> +# @ECLASS-VARIABLE: EBZR_UPDATE_CMD
>> +# @DESCRIPTION:
>> +# The Bazaar command to update the sources.
>> +: ${EBZR_UPDATE_CMD:="bzr pull --overwrite-tags"}
>> +
>> +# @ECLASS-VARIABLE: EBZR_EXPORT_CMD
>> +# @DESCRIPTION:
>> +# The Bazaar command to export a branch.
>> +: ${EBZR_EXPORT_CMD:="bzr export"}
>> +
>> +# @ECLASS-VARIABLE: EBZR_CHECKOUT_CMD
>> +# @DESCRIPTION:
>> +# The Bazaar command to checkout a branch.
>> +: ${EBZR_CHECKOUT_CMD:="bzr checkout --lightweight -q"}
>> +
>> +# @ECLASS-VARIABLE: EBZR_REVNO_CMD
>> +# @DESCRIPTION:
>> +# The Bazaar command to list a revision number of the branch.
>> +: ${EBZR_REVNO_CMD:="bzr revno"}

> Are you sure that having these overrides is a good idea?

Yes. Ebuilds had used them in the past.

> Your followup patch pretty much proves that every ebuild redefining
> them would get broken by switch to breezy.

The only one where syntax has changed is EBZR_INIT_REPO_CMD (which has
init-shared-repository instead of init-repository).

>> +# @ECLASS-VARIABLE: EBZR_OPTIONS
>> +# @DEFAULT_UNSET
>> +# @DESCRIPTION:
>> +# The options passed to the fetch and update commands.

> Is this intended to be set by ebuild or by user?

Ebuild.

>> +# @ECLASS-VARIABLE: EBZR_OFFLINE

> @USER_VARIABLE?

>> +# @ECLASS-VARIABLE: EVCS_UMASK

> @USER_VARIABLE?

Both added.

>> +# @FUNCTION: bzr_initial_fetch

> @INTERNAL?

Added, and renamed to _bzr_initial_fetch.

>> +	if [[ -n "${EBZR_OFFLINE}" ]]; then
>> +		ewarn "EBZR_OFFLINE cannot be used when there is no local branch yet."

> I dare say this is incorrect.  If user says "no online operations", then
> the eclass should just fail, not ignore the user.

Fixed.

>> +	${EBZR_FETCH_CMD} ${EBZR_OPTIONS} "${repo_uri}" "${branch_dir}" \
>> +		|| die "${EBZR}: can't branch from ${repo_uri}"

> You can replace the backslash with '||'.

I think that splitting the line before the operator is clearer. (It is
also what GNU coding standards recommend for C.)

>> +# @FUNCTION: bzr_update

> @INTERNAL?

Added, and renamed to _bzr_update.

>> +bzr_fetch() {
>> +	local repo_dir branch_dir
>> +	local save_sandbox_write=${SANDBOX_WRITE} save_umask
>> +
>> +	[[ -n ${EBZR_REPO_URI} ]] || die "${EBZR}: EBZR_REPO_URI is empty"
>> +
>> +	if [[ ! -d ${EBZR_STORE_DIR} ]] ; then
>> +		addwrite /
>> +		mkdir -p "${EBZR_STORE_DIR}" \
>> +			|| die "${EBZR}: can't mkdir ${EBZR_STORE_DIR}"
>> +		SANDBOX_WRITE=${save_sandbox_write}

> Looks like abusing implementation details.

Replaced by subshell.

>> +		if [[ -z ${EBZR_INITIAL_URI} ]]; then
>> +			bzr_initial_fetch "${EBZR_REPO_URI}" "${branch_dir}"
>> +		else
>> +			# Workaround for faster initial download. This clones the
>> +			# branch from a fast server (which may be out of date), and
>> +			# subsequently pulls from the slow original repository.
>> +			bzr_initial_fetch "${EBZR_INITIAL_URI}" "${branch_dir}"
>> +			if [[ ${EBZR_REPO_URI} != "${EBZR_INITIAL_URI}" ]]; then
>> +				EBZR_UPDATE_CMD="${EBZR_UPDATE_CMD} --remember --overwrite" \
>> +					EBZR_OFFLINE="" \

> Why do you override EBZR_OFFLINE here?

The whole else clause is dropped in a followup commit.

Ulrich