[gentoo-dev] bzr.eclass changes, please review

[gentoo-dev] bzr.eclass changes, please review

[Ulrich Mueller]

In bug 434746 it has been suggested that ${WORKDIR} should look like a
bzr branch or checkout. Proposed changes for bzr.eclass are included
below, comprising the following:

- bzr_fetch can optionally call bzr checkout --lightweight instead of
  bzr export. The default behaviour won't change, the new behaviour
  can be enabled with:
- New variable EBZR_WORKDIR_CHECKOUT, (Or can anyone come up with a
  better/shorter name?)
- New variable EBZR_CHECKOUT_CMD.
- The sandbox environment is now always restored; before it was only
  restored if ${EBZR_STORE_DIR} didn't exist. This is to prevent the
  package's build system from writing to ${EBZR_STORE_DIR} when it's
  calling bzr (which wasn't an issue for an export, but could be for
  a checkout).
- Unrelated to the above, some old cleanup code (around line 220) is
  removed.

The updated bzr.eclass is available in the emacs overlay, along with
an app-editors/emacs-vcs ebuild that I've used for testing.

Ulrich


--- bzr.eclass	18 Jul 2012 15:12:54 -0000	1.18
+++ bzr.eclass	14 Sep 2012 08:02:08 -0000
@@ -61,6 +61,11 @@
 # 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.
@@ -145,6 +150,12 @@
 # by users.
 : ${EBZR_OFFLINE=${EVCS_OFFLINE}}
 
+# @ECLASS-VARIABLE: EBZR_WORKDIR_CHECKOUT
+# @DEFAULT_UNSET
+# @DESCRIPTION:
+# If this variable is set to a non-empty value, EBZR_CHECKOUT_CMD will
+# be used instead of EBZR_EXPORT_CMD to copy the sources to WORKDIR.
+
 # @FUNCTION: bzr_initial_fetch
 # @USAGE: <repository URI> <branch directory>
 # @DESCRIPTION:
@@ -196,11 +207,11 @@
 # working copy.
 bzr_fetch() {
 	local repo_dir branch_dir
+	local save_sandbox_write=${SANDBOX_WRITE}
 
 	[[ -n ${EBZR_REPO_URI} ]] || die "${EBZR}: EBZR_REPO_URI is empty"
 
 	if [[ ! -d ${EBZR_STORE_DIR} ]] ; then
-		local save_sandbox_write=${SANDBOX_WRITE}
 		addwrite /
 		mkdir -p "${EBZR_STORE_DIR}" \
 			|| die "${EBZR}: can't mkdir ${EBZR_STORE_DIR}"
@@ -215,19 +226,6 @@
 
 	addwrite "${EBZR_STORE_DIR}"
 
-	# Clean up if the existing local copy is a checkout (as was the case
-	# with an older version of bzr.eclass).
-	# This test can be removed after 1 Mar 2012.
-	if [[ ${EBZR_FETCH_CMD} != *checkout* && -d ${repo_dir}/.bzr/checkout ]]
-	then
-		local tmpname=$(mktemp -u "${repo_dir}._old_.XXXXXX")
-		ewarn "checkout from old version of ${EBZR} found, moving it to:"
-		ewarn "${tmpname}"
-		ewarn "you may manually remove it"
-		mv "${repo_dir}" "${tmpname}" \
-			|| die "${EBZR}: can't move old checkout out of the way"
-	fi
-
 	if [[ ! -d ${branch_dir}/.bzr ]]; then
 		if [[ ${repo_dir} != "${branch_dir}" && ! -d ${repo_dir}/.bzr ]]; then
 			einfo "creating shared bzr repository: ${repo_dir}"
@@ -252,14 +250,23 @@
 		bzr_update "${EBZR_REPO_URI}" "${branch_dir}"
 	fi
 
+	# Restore sandbox environment
+	SANDBOX_WRITE=${save_sandbox_write}
+
 	cd "${branch_dir}" || die "${EBZR}: can't chdir to ${branch_dir}"
 
 	# Save revision number in environment. #311101
 	export EBZR_REVNO=$(${EBZR_REVNO_CMD})
 
-	einfo "exporting ..."
-	${EBZR_EXPORT_CMD} ${EBZR_REVISION:+-r ${EBZR_REVISION}} \
-		"${WORKDIR}/${P}" . || die "${EBZR}: export failed"
+	if [[ -n ${EBZR_WORKDIR_CHECKOUT} ]]; then
+		einfo "checking out ..."
+		${EBZR_CHECKOUT_CMD} ${EBZR_REVISION:+-r ${EBZR_REVISION}} \
+			. "${WORKDIR}/${P}" || die "${EBZR}: checkout failed"
+	else
+		einfo "exporting ..."
+		${EBZR_EXPORT_CMD} ${EBZR_REVISION:+-r ${EBZR_REVISION}} \
+			"${WORKDIR}/${P}" . || die "${EBZR}: export failed"
+	fi
 	einfo "revision ${EBZR_REVISION:-${EBZR_REVNO}} is now in ${WORKDIR}/${P}"
 
 	popd > /dev/null

Re: [gentoo-dev] bzr.eclass changes, please review

[Rick "Zero_Chaos" Farina]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/14/2012 05:01 AM, Ulrich Mueller wrote:
> In bug 434746 it has been suggested that ${WORKDIR} should look like a
> bzr branch or checkout. Proposed changes for bzr.eclass are included
> below, comprising the following:
> 
> - bzr_fetch can optionally call bzr checkout --lightweight instead of
>   bzr export. The default behaviour won't change, the new behaviour
>   can be enabled with:
> - New variable EBZR_WORKDIR_CHECKOUT, (Or can anyone come up with a
>   better/shorter name?)
> - New variable EBZR_CHECKOUT_CMD.
> - The sandbox environment is now always restored; before it was only
>   restored if ${EBZR_STORE_DIR} didn't exist. This is to prevent the
>   package's build system from writing to ${EBZR_STORE_DIR} when it's
>   calling bzr (which wasn't an issue for an export, but could be for
>   a checkout).
> - Unrelated to the above, some old cleanup code (around line 220) is
>   removed.
> 
> The updated bzr.eclass is available in the emacs overlay, along with
> an app-editors/emacs-vcs ebuild that I've used for testing.
> 
> Ulrich
> 
> 
> --- bzr.eclass	18 Jul 2012 15:12:54 -0000	1.18
> +++ bzr.eclass	14 Sep 2012 08:02:08 -0000
> @@ -61,6 +61,11 @@
>  # 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.
> @@ -145,6 +150,12 @@
>  # by users.
>  : ${EBZR_OFFLINE=${EVCS_OFFLINE}}
>  
> +# @ECLASS-VARIABLE: EBZR_WORKDIR_CHECKOUT
> +# @DEFAULT_UNSET
> +# @DESCRIPTION:
> +# If this variable is set to a non-empty value, EBZR_CHECKOUT_CMD will
> +# be used instead of EBZR_EXPORT_CMD to copy the sources to WORKDIR.
> +
>  # @FUNCTION: bzr_initial_fetch
>  # @USAGE: <repository URI> <branch directory>
>  # @DESCRIPTION:
> @@ -196,11 +207,11 @@
>  # working copy.
>  bzr_fetch() {
>  	local repo_dir branch_dir
> +	local save_sandbox_write=${SANDBOX_WRITE}
>  
>  	[[ -n ${EBZR_REPO_URI} ]] || die "${EBZR}: EBZR_REPO_URI is empty"
>  
>  	if [[ ! -d ${EBZR_STORE_DIR} ]] ; then
> -		local save_sandbox_write=${SANDBOX_WRITE}
>  		addwrite /
Am I reading this wrong, or is "addwrite /" being more than just a
little lazy?  I know this isn't part of your change set but it has
always bothered me that it needs to unlocking writing on the whole
filesystem to save something in distdir.

Thanks,
Zero
>  		mkdir -p "${EBZR_STORE_DIR}" \
>  			|| die "${EBZR}: can't mkdir ${EBZR_STORE_DIR}"
> @@ -215,19 +226,6 @@
>  
>  	addwrite "${EBZR_STORE_DIR}"
>  
> -	# Clean up if the existing local copy is a checkout (as was the case
> -	# with an older version of bzr.eclass).
> -	# This test can be removed after 1 Mar 2012.
> -	if [[ ${EBZR_FETCH_CMD} != *checkout* && -d ${repo_dir}/.bzr/checkout ]]
> -	then
> -		local tmpname=$(mktemp -u "${repo_dir}._old_.XXXXXX")
> -		ewarn "checkout from old version of ${EBZR} found, moving it to:"
> -		ewarn "${tmpname}"
> -		ewarn "you may manually remove it"
> -		mv "${repo_dir}" "${tmpname}" \
> -			|| die "${EBZR}: can't move old checkout out of the way"
> -	fi
> -
>  	if [[ ! -d ${branch_dir}/.bzr ]]; then
>  		if [[ ${repo_dir} != "${branch_dir}" && ! -d ${repo_dir}/.bzr ]]; then
>  			einfo "creating shared bzr repository: ${repo_dir}"
> @@ -252,14 +250,23 @@
>  		bzr_update "${EBZR_REPO_URI}" "${branch_dir}"
>  	fi
>  
> +	# Restore sandbox environment
> +	SANDBOX_WRITE=${save_sandbox_write}
> +
>  	cd "${branch_dir}" || die "${EBZR}: can't chdir to ${branch_dir}"
>  
>  	# Save revision number in environment. #311101
>  	export EBZR_REVNO=$(${EBZR_REVNO_CMD})
>  
> -	einfo "exporting ..."
> -	${EBZR_EXPORT_CMD} ${EBZR_REVISION:+-r ${EBZR_REVISION}} \
> -		"${WORKDIR}/${P}" . || die "${EBZR}: export failed"
> +	if [[ -n ${EBZR_WORKDIR_CHECKOUT} ]]; then
> +		einfo "checking out ..."
> +		${EBZR_CHECKOUT_CMD} ${EBZR_REVISION:+-r ${EBZR_REVISION}} \
> +			. "${WORKDIR}/${P}" || die "${EBZR}: checkout failed"
> +	else
> +		einfo "exporting ..."
> +		${EBZR_EXPORT_CMD} ${EBZR_REVISION:+-r ${EBZR_REVISION}} \
> +			"${WORKDIR}/${P}" . || die "${EBZR}: export failed"
> +	fi
>  	einfo "revision ${EBZR_REVISION:-${EBZR_REVNO}} is now in ${WORKDIR}/${P}"
>  
>  	popd > /dev/null
> 
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQUy3iAAoJEKXdFCfdEflKGn8P/2nWVsOWor82RWYneDHpxaFU
U0rZn7D3PPNCLhNpIH9/61BlJsarOW7MuFSgTwTRlAnyKN8gLMyOYvr5bBi9URgT
7rC6pKxy+tH9EoyemZks9SoTjSQk6d+jZLx+0nvFUqvqtK+Elp0qs4FyxyWDqq0j
ZL3dunSIcNHhekQYbuaIJQ6bCBTUyqtUTLyzIWRveoHaGPE0Nl5r7y/LJOeOA4JX
my+3UyK6snWvpuUqYjEXjTTRvZivoD1hoX9ALtFOFquzQ6ITHYAWSYgfGP96cTAo
+IiSt2DgSCz1wnGyXPgRgj7I0cijtBZ/ozkIZVfEAdbyzatVkOh+vkepF3Fw8698
1V5SDjqqPCEtHTcLEvTD5Q7yUhx805N78v47GsYAdwul3ZCFCW8nNUdzbCEIQXWD
QXwOZQ9wsYFhedJgbO6Sd68au4OP4L1yL+u/+wdyag5ipnBRcCMuInYmL8n2Orkq
UZR+XI6QURauCqg/MoaiUcbRbE6uUQarke93uU0hR3odZlBDk3Evo9Vvs+veK5Cq
a9VJig3PD+b+EjFfenDlwptI3oBXtd0NMZgkqL3NrOAxsz4osDj7Un9TXqbdHHt7
LAlIgAxWjGZBjGDoj+uFT5DBb+bhI2g0XHik96O5+47oDqw4VORtdOyo0/oo1p14
FdwcD7TA9Xb/WDqCKicg
=dB/J
-----END PGP SIGNATURE-----

Re: [gentoo-dev] bzr.eclass changes, please review

[Ulrich Mueller]

>>>>> On Fri, 14 Sep 2012, Rick "Zero Chaos" Farina wrote:

>>  		addwrite /
> Am I reading this wrong, or is "addwrite /" being more than just a
> little lazy?  I know this isn't part of your change set but it has
> always bothered me that it needs to unlocking writing on the whole
> filesystem to save something in distdir.

EBZR_STORE_DIR may be redefined, so you cannot be sure that it's below
DISTDIR. Also, it's not known how many of its path components already
exist. I'm not sure if it would be worth the effort to compute a more
accurate argument for addwrite. (Almost all live eclasses do that
"addwrite /", by the way.)

>>  		mkdir -p "${EBZR_STORE_DIR}" \
>>  			|| die "${EBZR}: can't mkdir ${EBZR_STORE_DIR}"

Ulrich

Re: [gentoo-dev] bzr.eclass changes, please review

[Rick "Zero_Chaos" Farina]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/14/2012 11:51 AM, Ulrich Mueller wrote:
>>>>>> On Fri, 14 Sep 2012, Rick "Zero Chaos" Farina wrote:
> 
>>>  		addwrite /
>> Am I reading this wrong, or is "addwrite /" being more than just a
>> little lazy?  I know this isn't part of your change set but it has
>> always bothered me that it needs to unlocking writing on the whole
>> filesystem to save something in distdir.
> 
> EBZR_STORE_DIR may be redefined, so you cannot be sure that it's below
> DISTDIR. Also, it's not known how many of its path components already
> exist. I'm not sure if it would be worth the effort to compute a more
> accurate argument for addwrite. (Almost all live eclasses do that
> "addwrite /", by the way.)
I didn't mean to pick on bzr.eclass, I think it's always wrong to do
this.  And you picked out the exact reasoning I did "I'm not sure if it
would be worth the effort to compute a more accurate argument for
addwrite." I think it is worth the effort to do it right.  I mean
(purposeful exaggeration here) we could save the addwrite entirely by
just "killall sandbox" or we could prevent from reoccurring by
restricting the sandbox feature. Any time you do "addwrite /" you
completely defeat the entire purpose of sandbox.  It's not write (get it?).

I'm not saying this is an emergency nor should it hold back any changes
you need to make to argue with me about it. However, if you were to do
it right that would be cool. Otherwise we could all start fixing our
sandbox issues by just doing "addwrite /" at the top of all ebuilds.

Thanks,
Zero
> 
>>>  		mkdir -p "${EBZR_STORE_DIR}" \
>>>  			|| die "${EBZR}: can't mkdir ${EBZR_STORE_DIR}"
> 
> Ulrich
> 
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQU1djAAoJEKXdFCfdEflKlecP/jVjGN76yb/VJ+5/O75lwwhf
7p7pppohVw4gfaiiHOz0uamu37RPPqlmc+LV4TgUxQ+n6VMkdrXG7tU0TTewYEEw
mTorfsF5i2aFXJXMRIT5Xz+P+iEx9m4bhbKuxjL17DKs4F9jKToao/1ix2lhNKZX
ClnAp+qgKNN2fj/1vnBNkGSIfWF9EEkpfCqC0d2kXLADXJCNqaOmUmVI9+5XXAFw
7X+DW1JMRgg64yQ78bztHmmKqQ211WK3JqqmZPxb4bms5RQPCILBjc+OQ97KKwBW
woXfV+gZInTrfvgADEFCLeDSKuEojqriu7UrXCBkwNkDEzVdsLWXD25lUzo2bmPP
nuUOjBsIiFJAt8UdpAn+y7hDVy5BfClE24FxVSk+ydthlkGNW6T0tMEikBhserJ5
bi0qF7qOp6Wu+OVS0a+de2ptcy6z/AVm8ziDSY70mX32GqW0APkft+yvzluibpXZ
a37c+zYoytWb1GK5ijC8I29xi5GDilouaX+DMVz2woZChEhYuZ8ElKpHzHgI1HZs
dZirnsFjZt4jBBk2iB8NUHyccze4XpmSnwd75w0ltNaAs1IhTATxIbbkRokQJ0F7
FteSyd/0jzvRGIIgpTpJBtga4UMkRZxpnG/2OIKn0hy+4tvotwUknwzWDGEdCKQq
5k4jnZXA364VLoeYOXeK
=0AWM
-----END PGP SIGNATURE-----

Re: [gentoo-dev] bzr.eclass changes, please review

[Mike Gilbert]

On Fri, Sep 14, 2012 at 12:12 PM, Rick "Zero_Chaos" Farina
<zerochaos@gentoo.org> wrote:
> I didn't mean to pick on bzr.eclass, I think it's always wrong to do
> this.  And you picked out the exact reasoning I did "I'm not sure if it
> would be worth the effort to compute a more accurate argument for
> addwrite." I think it is worth the effort to do it right.  I mean
> (purposeful exaggeration here) we could save the addwrite entirely by
> just "killall sandbox" or we could prevent from reoccurring by
> restricting the sandbox feature. Any time you do "addwrite /" you
> completely defeat the entire purpose of sandbox.  It's not write (get it?).
>
> I'm not saying this is an emergency nor should it hold back any changes
> you need to make to argue with me about it. However, if you were to do
> it right that would be cool. Otherwise we could all start fixing our
> sandbox issues by just doing "addwrite /" at the top of all ebuilds.
>

The sandbox is mostly useful to prevent build systems from messing
with the live system without the developer's knowledge. It is
perfectly reasonable to disable the sandbox for a single mkdir call
that we have direct control over.