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
09-14-2012, 01:15 PM
"Rick "Zero_Chaos" Farina"
bzr.eclass changes, please review
-----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/
>>>>> 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.)
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/
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.
bzr.eclass changes, please review
