cros_workon: modify regen_manifest_and_sync to use loman

cros_workon: modify regen_manifest_and_sync to use loman

This fixes a bug where user added local_manifest.xml entries
get clobbered.

BUG=5787
TEST=Verified start of various packages works correctly.

Change-Id: I2348dfb1be098b81ede5928426192c655160564d

[anush, 2010-09-16 17:31:50]

LGTM.  Please update the TEST= with the fact that it didn't clobber existing
local_manifest.xml. As we spoke please verify crosutils stable is uprev'd to
include loman in the chroot. (hard-host has since been uprev'd already)

Thanks.



On Thu, Sep 16, 2010 at 9:58 AM, <msb@chromium.org> wrote:

> Reviewers: anush,
>
> Description:
> cros_workon: modify regen_manifest_and_sync to use loman
>
> This fixes a bug where user added local_manifest.xml entries
> get clobbered.
>
> BUG=5787
> TEST=Verified start of various packages works correctly.
>
> Change-Id: I2348dfb1be098b81ede5928426192c655160564d
>
> Please review this at http://codereview.chromium.org/3389013/show
>
> SVN Base: http://git.chromium.org/git/crosutils.git
>
> Affected files:
>  M cros_workon
>
>
> Index: cros_workon
> diff --git a/cros_workon b/cros_workon
> index
>
d48038ee148e58e2b5c87e3ddb4ca7fad74d59bf..59e1bf0b293686ebf24beb1d9715a44ef864f6f9
> 100755
> --- a/cros_workon
> +++ b/cros_workon
> @@ -153,55 +153,15 @@ find_repo_dir () {
>
>  # This is called only for "cros-workon start". We dont handle the "stop"
> case since the local changes are ignored anyway since the 9999.ebuild is
> masked and we dont want to deal with what to do with the user's local
> changes.
>  regen_manifest_and_sync() {
> -  find_repo_dir
> -  local_manifest="${REPODIR}/local_manifest.xml"
> -
> -  # ensure we don't clobber existing manifest entries
> -  if [ -f "${local_manifest}" ]; then
> -    grep -q -i "<remote" "${local_manifest}"
> -    if [ $? -ne 1 ]; then
> -      die "Your local manifest will be clobbered running cros_workon. You
> can not have any <remote> tags at all."
> -    fi
> -    sed -ne '/^[[:space:]]\+[^[:space:]]/q1' "${local_manifest}"
> -    if [ $? -ne 0 ]; then
> -      die "Your local manifest will be clobbered running cros_workon. You
> can not have any tags that span multiple lines or are indented."
> -    fi
> -    egrep -q "^[^<]" "${local_manifest}"
> -    if [ $? -ne 1 ]; then
> -      die "Your local manifest will be clobbered running cros_workon. You
> can not have any tags that span multiple lines"
> -    fi
> -    grep -v "<project" "${local_manifest}" | grep -q -i "<project"
> -    if [ $? -eq 0 ]; then
> -      die "Your local manifest has mixed case tags for project.  You can
> not have mixed case tags"
> -    fi
> -  fi
> -  # preserve old manifest entries
> -  [ -f "${local_manifest}" ] && \
> -    MANIFEST_ENTRIES_OLD=$(cat "${local_manifest}" | grep "^<project")
> -
> -  rm -f "${local_manifest}"
> -
> -  # get new manifest entries
> -  MANIFEST_ENTRIES=$(show_live_ebuilds |
> -  {
> -    while read line
> -    do
> -      pkgname=`basename ${line}`
> -      eval $(${EBUILDCMD} $(${EQUERYCMD} which ${pkgname}) info)
> -      REPO_ELEMENT=$(sed -n '/START_MINILAYOUT/,/STOP_MINILAYOUT/p'
> $REPODIR/manifest.xml | grep "name=\"${CROS_WORKON_PROJECT}\"" | sed -e
> 's/^[ \t]*//')
> -      echo "$REPO_ELEMENT"
> -    done
> -  })
> -
> -  if [ -n "${MANIFEST_ENTRIES}" ] || [ -n "${MANIFEST_ENTRIES_OLD}" ];
> then
> -    info "Creating local manifest for workon packages.."
> -    echo "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
>
>> "${local_manifest}"
>>>
>> -    echo "<manifest>" >> "${local_manifest}"
> -    echo -e "${MANIFEST_ENTRIES}\n${MANIFEST_ENTRIES_OLD}" \
> -      | sort | uniq >> "${local_manifest}"
> -    echo "</manifest>" >> "${local_manifest}"
> -    echo "Please run \"repo sync\" now."
> -  fi
> +  for pkgname in $(show_live_ebuilds); do
> +    eval $(${EBUILDCMD} $(${EQUERYCMD} which ${pkgname}) info)
> +    local srcdir=$(readlink -f ${CROS_WORKON_SRCDIR})
> +    local trunkdir=$(readlink -f ${CHROOT_TRUNK_DIR})
> +    local path=${srcdir#$(readlink -f ${CHROOT_TRUNK_DIR})/}
> +
> +    loman add --workon "${CROS_WORKON_PROJECT}" "${path}"
> +  done
> +  echo "Please run \"repo sync\" now."
>  }
>
>  # Move a stable ebuild to the live development catgeory.  The ebuild
>
>
>

[kliegs, 2010-09-16 17:44:02]

LGTM with nit

Also would like to see more details on the TEST in the future. As example:

Ran cros_workon for a repo that already existed in local_manifest
Ran cros_workon for a repo that didn't
Ran cros_workon against an empty local_manifest
Ran cros_workon against a local_manifest that contained other remotes defined
Ran cros_workon against a local_manifest with comments
Ran cros_workon against a local_manifest which wasn't valid XML

Although I think most of this testing was done in testing loman itself so
referencing those tests should be fine.

http://codereview.chromium.org/3389013/diff/1/2
File cros_workon (right):

http://codereview.chromium.org/3389013/diff/1/2#newcode160
cros_workon:160: local path=${srcdir#$(readlink -f ${CHROOT_TRUNK_DIR})/}
Using path here can cause confusion with the environment variable PATH.  I'd
prefer to see a different name

[Mandeep Singh Baines, 2010-09-16 17:58:56]

Fixed nit. Retested. Committed.

kliegs@chromium.org (kliegs@chromium.org) wrote:
> LGTM with nit
> 
> Also would like to see more details on the TEST in the future. As example:
> 
> Ran cros_workon for a repo that already existed in local_manifest
> Ran cros_workon for a repo that didn't
> Ran cros_workon against an empty local_manifest
> Ran cros_workon against a local_manifest that contained other
> remotes defined
> Ran cros_workon against a local_manifest with comments
> Ran cros_workon against a local_manifest which wasn't valid XML
> 
> Although I think most of this testing was done in testing loman itself so
> referencing those tests should be fine.
> 
> 
> http://codereview.chromium.org/3389013/diff/1/2
> File cros_workon (right):
> 
> http://codereview.chromium.org/3389013/diff/1/2#newcode160
> cros_workon:160: local path=${srcdir#$(readlink -f
> ${CHROOT_TRUNK_DIR})/}
> Using path here can cause confusion with the environment variable PATH.
> I'd prefer to see a different name
> 
> http://codereview.chromium.org/3389013/show