cros_workon: initial implementation

cros_workon: initial implementation
    
Moves ebuild(s) between stable and live states.
    
Lists ebuilds currently under development in the testing branch.
    
Stable ebuilds obtain and build against the last known good commit.
Live ebuilds (9999) perform a 'git clone' during the src_unpack
step to obtain and save the source for development and building.
    
Example usage:
    
  Sync & build 'ibus-hangul' testing ebuild (9999)
     ./cros_workon start app-i18n/ibus-hangul
    
  Build 'ibus-hangul' from the last stable commit
     ./cros_workon stop app-i18n/ibus-hangul
    
  List of ebuilds under development
     ./cros_workon list
    
Change-Id: I2ea4babd7597d5cea9ca96419a74152f9f0b23f1
    
Note: --board must be specified if the --default option
      to setup_board was not used.

[sosa, 2010-06-22 20:28:24]

Some meta-comments:

I don't like using the word "testing" when talking about working on ebuilds. 
This is already on overloaded term that I don't want to overload too much more.

Use spaces instead of tabs everywhere.

Use periods at the end of comments.

I'm not sure about your choice of using text files in
/board/etc/portage/{component}.  Why not store them all in the same file?  Or
store the whole path as a file i.e. {component}/{package} as a file like portage
does in /var.

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

http://codereview.chromium.org/2852019/diff/1/2#newcode7
cros_workon:7: # Moves ebuilds between stable to testing branches. List test
branch ebuilds.
Can you give a higher-level comment.  This is too detailed.

http://codereview.chromium.org/2852019/diff/1/2#newcode7
cros_workon:7: # Moves ebuilds between stable to testing branches. List test
branch ebuilds.
I prefer using the world unstable rather than test.  Using test mucks this
around too much with other actual test things.

http://codereview.chromium.org/2852019/diff/1/2#newcode30
cros_workon:30: DEFINE_string stop "" \
Alphabetize

http://codereview.chromium.org/2852019/diff/1/2#newcode39
cros_workon:39: 
Too many lines separating

http://codereview.chromium.org/2852019/diff/1/2#newcode42
cros_workon:42: error "--board is required."
Use die instead of error and remove the exit 1.  Also, only indent with 2
spaces.

http://codereview.chromium.org/2852019/diff/1/2#newcode48
cros_workon:48: 
Too many lines separating

http://codereview.chromium.org/2852019/diff/1/2#newcode54
cros_workon:54: local atoms=$1
Use spaces not tabs in bash scripts that are not portage ebuilds.

http://codereview.chromium.org/2852019/diff/1/2#newcode67
cros_workon:67: `grep "${package}" "${KEYWORDS_DIR}/${component}"` ]]; then
Use grep -q

http://codereview.chromium.org/2852019/diff/1/2#newcode68
cros_workon:68: echo "   '${component}/${package}' already marked for
development"
Shouldn't this be a warning instead of an echo?

http://codereview.chromium.org/2852019/diff/1/2#newcode70
cros_workon:70: fi
Use if/else instead of if - continue

http://codereview.chromium.org/2852019/diff/1/2#newcode73
cros_workon:73: sudo bash -c "echo \"${component}/${package}\" >>
\"${KEYWORDS_DIR}/${component}\""
Isn't storing the component redundant at this stage?

http://codereview.chromium.org/2852019/diff/1/2#newcode77
cros_workon:77: # Move a test branch ebuild back to stable
Period at end of all comments.

http://codereview.chromium.org/2852019/diff/1/2#newcode99
cros_workon:99: sudo find "${KEYWORDS_DIR}/" -type f -empty -delete
Do you need sudo for find?

http://codereview.chromium.org/2852019/diff/1/2#newcode111
cros_workon:111: if [[ -n ${FLAGS_start} ]]; then
Put ""'s around FLAGS_start

http://codereview.chromium.org/2852019/diff/1/2#newcode115
cros_workon:115: if [[ -n ${FLAGS_stop} ]]; then
Put ""'s around stop

[djmm, 2010-06-22 21:05:05]

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

http://codereview.chromium.org/2852019/diff/1/2#newcode67
cros_workon:67: `grep "${package}" "${KEYWORDS_DIR}/${component}"` ]]; then
Use $() construct (rather than backticks) for encapsulating command execution.

http://codereview.chromium.org/2852019/diff/1/2#newcode67
cros_workon:67: `grep "${package}" "${KEYWORDS_DIR}/${component}"` ]]; then
Probably need grep -qw to eliminate ambiguity.

[brian d, 2010-06-22 21:43:57]

Thanks for the great feedback!

Testing will be called Live... this should avoid naming collisions and confusion
with existing Portage terms as well.

Storing the 'component/package atom' name in a file in the package.keywords is a
bit "cleaner". Arguable though -- 1 vs many.
One justification was that it's easier to find and maintain component files
rather than a single large package.keywords file.

The use case for package.keywords being a directory is to allow cros_workon to
update it's files and for developers to twiddle with their own and not have them
collide.


On 2010/06/22 20:28:24, sosa wrote:
> Some meta-comments:
> 
> I don't like using the word "testing" when talking about working on ebuilds. 
> This is already on overloaded term that I don't want to overload too much
more.
> 
> Use spaces instead of tabs everywhere.
> 
> Use periods at the end of comments.
> 
> I'm not sure about your choice of using text files in
> /board/etc/portage/{component}.  Why not store them all in the same file?  Or
> store the whole path as a file i.e. {component}/{package} as a file like
portage
> does in /var.
> 
> http://codereview.chromium.org/2852019/diff/1/2
> File cros_workon (right):
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode7
> cros_workon:7: # Moves ebuilds between stable to testing branches. List test
> branch ebuilds.
> Can you give a higher-level comment.  This is too detailed.
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode7
> cros_workon:7: # Moves ebuilds between stable to testing branches. List test
> branch ebuilds.
> I prefer using the world unstable rather than test.  Using test mucks this
> around too much with other actual test things.
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode30
> cros_workon:30: DEFINE_string stop "" \
> Alphabetize
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode39
> cros_workon:39: 
> Too many lines separating
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode42
> cros_workon:42: error "--board is required."
> Use die instead of error and remove the exit 1.  Also, only indent with 2
> spaces.
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode48
> cros_workon:48: 
> Too many lines separating
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode54
> cros_workon:54: local atoms=$1
> Use spaces not tabs in bash scripts that are not portage ebuilds.
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode67
> cros_workon:67: `grep "${package}" "${KEYWORDS_DIR}/${component}"` ]]; then
> Use grep -q
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode68
> cros_workon:68: echo "   '${component}/${package}' already marked for
> development"
> Shouldn't this be a warning instead of an echo?
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode70
> cros_workon:70: fi
> Use if/else instead of if - continue
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode73
> cros_workon:73: sudo bash -c "echo \"${component}/${package}\" >>
> \"${KEYWORDS_DIR}/${component}\""
> Isn't storing the component redundant at this stage?
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode77
> cros_workon:77: # Move a test branch ebuild back to stable
> Period at end of all comments.
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode99
> cros_workon:99: sudo find "${KEYWORDS_DIR}/" -type f -empty -delete
> Do you need sudo for find?
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode111
> cros_workon:111: if [[ -n ${FLAGS_start} ]]; then
> Put ""'s around FLAGS_start
> 
> http://codereview.chromium.org/2852019/diff/1/2#newcode115
> cros_workon:115: if [[ -n ${FLAGS_stop} ]]; then
> Put ""'s around stop

[Mandeep Singh Baines, 2010-06-22 23:34:10]

Instead of having a file in package.keywords per atom, have a single file named
cros-workon.

From man  portage:


       /etc/portage/
              Any file in this directory that begins with "package." can
              be more than just a flat file.  If it is a directory, then
              all  the files in that directory will be sorted in ascend-
              ing alphabetical order by file name and summed together as
              if it were a single file.

              Example:
              /etc/portage/package.keywords/common
              /etc/portage/package.keywords/e17
              /etc/portage/package.keywords/kde

[brian d, 2010-06-22 23:35:00]

Updated with review and design feedback.

[zbehan, 2010-06-22 23:51:08]

On 2010/06/22 21:43:57, bdaugher wrote:
> Testing will be called Live... this should avoid naming collisions and
confusion
> with existing Portage terms as well.

To be absolutely fair, the Portage team also uses the term "live ebuild". It
describes ebuilds that pull sources from a repo rather than fetch a source
tarball, and are usually versioned -9999, which is where our versioning scheme
comes from.

In fact, this naming scheme also collides, because for chromeos, the _stable_
ebuilds pull sources from a known good revision of repo, while the 9999 ebuilds
pick up a live copy that you're working on from somewhere on your disk.

Anyway, it's probably more important to make our terminology comprehensible than
to make sure we don't collide with anything. (Portage team already claimed all
the good things)

[sosa, 2010-06-23 00:12:55]

Mostly nits.

Other comments:
You have a lot of echo's in this script.  You may want to hide them under a
verbose flag.

http://codereview.chromium.org/2852019/diff/3/10002
File cros_workon (right):

http://codereview.chromium.org/2852019/diff/3/10002#newcode22
cros_workon:22: WORKON_CMD=$1
This has some issues if you use restart_in_chroot_if_need and it needs to
restart in the chroot.  You've already shifted so you'll will re-shift when you
restart the script.  Run restart before those.

http://codereview.chromium.org/2852019/diff/3/10002#newcode64
cros_workon:64: 
Remove extra line.

http://codereview.chromium.org/2852019/diff/3/10002#newcode67
cros_workon:67: else
Should match up with if.

http://codereview.chromium.org/2852019/diff/3/10002#newcode68
cros_workon:68: 
Remove this extra line.

http://codereview.chromium.org/2852019/diff/3/10002#newcode90
cros_workon:90: 
Remove extra line.

http://codereview.chromium.org/2852019/diff/3/10002#newcode92
cros_workon:92: sudo bash -c "sed \"/${package}/d\"
\"${KEYWORDS_DIR}/${component}\" > \"${KEYWORDS_DIR}/${component}+\""
Line should not be > 80 char.  Use \ to continue line

[brian d, 2010-06-23 16:42:16]

Updated with latest feedback.

[sosa, 2010-06-23 18:20:16]

Few last comments to address then LGTM as long as other reviewers don't have
other comments.

http://codereview.chromium.org/2852019/diff/17001/18001
File cros_workon (right):

http://codereview.chromium.org/2852019/diff/17001/18001#newcode26
cros_workon:26: shift
Put this below FLAGS "@" otherwise this only works if you use cros_workon CMD
--board board ... instead of letting --board {board} be anywhere.

http://codereview.chromium.org/2852019/diff/17001/18001#newcode28
cros_workon:28: get_default_board
Put this with restart in chroot

http://codereview.chromium.org/2852019/diff/17001/18001#newcode92
cros_workon:92: # Display ebuilds currently part of the live branch and open for
developpment.
Misspelling of development

[Mandeep Singh Baines, 2010-06-23 20:13:01]

LGTM

Tiny nit which is not necessary to address.

http://codereview.chromium.org/2852019/diff/17001/18001
File cros_workon (right):

http://codereview.chromium.org/2852019/diff/17001/18001#newcode85
cros_workon:85: sudo bash -c "sed -e 's|${atom}||' -e '/^$/d' <
\"${KEYWORDS_FILE}\" \
Might be simpler as:

sudo bash -c "grep -v '^${atom}$' \"${KEYWORDS_FILE}\" ...

[Mandeep Singh Baines, 2010-06-23 20:44:51]

LGTM