Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2071243002:
infra/go: Adding setup instructions and quick setup script. (Closed)
Descriptioninfra/go: Adding setup instructions and quick setup script.
The setup instructions explain how to get the "Chromium Infra Go Area" and the
quick setup script gets you started with one command and provides a wrapper.
This was split out of https://codereview.chromium.org/1807463005/
Committed: https://chromium.googlesource.com/infra/infra/+/e8441f571e7a2198e7837bb03b147077e23df7bf
Patch Set 1 #
Total comments: 30
Patch Set 2 : Review fixes. #
Total comments: 19
Patch Set 3 : Fixing for review. #Patch Set 4 : Need to export PS1. #Patch Set 5 : cd into the source area. #
Total comments: 1
Messages
Total messages: 36 (10 generated)
tansell@chromium.org changed reviewers: + maruel@chromium.org, tandrii@chromium.org
Hi, I've moved the quicksetup script from the luci-go repo to this infra/go repository and updated the instructions. See the discussion in https://codereview.chromium.org/1807463005/ Once this lands, I plan to update the README in luci-go to reference this stuff. Tim 'mithro' Ansell
tandrii@chromium.org changed reviewers: + vadimsh@chromium.org
+Vadim as the guardian of infra/go area LGTM from me. https://codereview.chromium.org/2071243002/diff/1/go/README.md File go/README.md (right): https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode13 go/README.md:13: ### Quick Setup s/Quick Setup/I just want to start coding (aka i just want to serve 5TB) https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode17 go/README.md:17: ```shell This works fine for github flavor of MD, but I'm not sure about Gerrit.
https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh File go/quicksetup.sh (right): https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode17 go/quicksetup.sh:17: "$PWD/depot_tools/fetch" infra I've just tried it with existing and dirty checkout of depot_tools already in Path. Your script hangs ~2 minutes because `fetch` command runs `gclient` not form the same depot_tools as `fetch` is, but from whatever is on $PATH. So, how about exporting new $PATH prior to running fetch? export PATH="$PWD/depot_tools:$PATH" fetch infra https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode33 go/quicksetup.sh:33: export PS1="[cr-infra-go-area] \$PS1" very nice! https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode46 go/quicksetup.sh:46: # Output usage instructions add extra echo here, so that instruction of what to run next stands out from lots of output above. https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode48 go/quicksetup.sh:48: ln -sf $ENTER_SCRIPT ~/bin/cr-infra-go-area-enter can we bail instead of overwriting existing file if any? Alternatively maybe write at the top of the script (~line 23) "DO NOT MODIFY, automatically generated, will be overwritten"
https://codereview.chromium.org/2071243002/diff/1/go/README.md File go/README.md (right): https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode17 go/README.md:17: ```shell On 2016/06/17 09:28:44, tandrii(chromium) wrote: > This works fine for github flavor of MD, but I'm not sure about Gerrit. It does work in Gerrit too: https://chromium.googlesource.com/infra/infra/+/3b59c550ed9d6a1e73c9451bad5e8... (I've uploaded your CL to gerrit with "git cl upload --gerrit" https://chromium-review.googlesource.com/c/353571/)
ericburnett@google.com changed reviewers: + ericburnett@google.com
Commenting as a target for this kind of script/documentation. Thanks for adding it, I was just figuring this out for myself :). https://codereview.chromium.org/2071243002/diff/1/go/README.md File go/README.md (right): https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode7 go/README.md:7: The steps for getting the code are; ';' -> ':' https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode15 go/README.md:15: If you are on Linux you can run the [quicksetup script](quicksetup.sh) like so; ';' -> ':' https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode20 go/README.md:20: ``` Can you also add a comment explaining what this will do? Something like: "This will create a self-contained cr-infra-go-area directory and populate it will all necessary tools and source for using or contributing to Chromium's Go Infrastructure. Once run, look in cr-infra-go-area/infra/go/src for the editable source code." https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh File go/quicksetup.sh (right): https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode48 go/quicksetup.sh:48: ln -sf $ENTER_SCRIPT ~/bin/cr-infra-go-area-enter Up until here the created environment was self-contained, in that it had no persistent impacts for anything outside cr-infra-go-area. I don't like the idea of adding things to the users' bin directory, as that breaks the containment (cannot be undone by just rm -rf cr-infra-go-area; installing again in a different path will collide on the symlink). Can you stop at printing $ENTER_SCRIPT, and leave it to the user to symlink it for their own benefit under whatever name they like? https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode54 go/quicksetup.sh:54: echo "Enter the environment by running '$PWD/enter-env.sh'" Reference $ENTER_SCRIPT here for consistency?
https://codereview.chromium.org/2071243002/diff/1/go/README.md File go/README.md (right): https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode19 go/README.md:19: wget -O- "https://chromium.googlesource.com/infra/infra/+/master/go/quicksetup.sh?format=TEXT" | bash format=TEXT actually returns base64 encoded text :-/ e.g. https://chromium.googlesource.com/infra/infra/+/master/go/check.py?format=TEXT (that's how Gerrit works around XSS possibility). https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh File go/quicksetup.sh (right): https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode33 go/quicksetup.sh:33: export PS1="[cr-infra-go-area] \$PS1" we should add it to go/env.py instead, probably, not everyone will be using quicksetup.sh https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode48 go/quicksetup.sh:48: ln -sf $ENTER_SCRIPT ~/bin/cr-infra-go-area-enter On 2016/06/17 17:23:31, Eric Burnett wrote: > Up until here the created environment was self-contained, in that it had no > persistent impacts for anything outside cr-infra-go-area. I don't like the idea > of adding things to the users' bin directory, as that breaks the containment > (cannot be undone by just rm -rf cr-infra-go-area; installing again in a > different path will collide on the symlink). > > Can you stop at printing $ENTER_SCRIPT, and leave it to the user to symlink it > for their own benefit under whatever name they like? +1
PTAL. https://codereview.chromium.org/2071243002/diff/1/go/README.md File go/README.md (right): https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode7 go/README.md:7: The steps for getting the code are; On 2016/06/17 17:23:30, Eric Burnett wrote: > ';' -> ':' Done. https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode13 go/README.md:13: ### Quick Setup On 2016/06/17 09:28:44, tandrii(chromium) wrote: > s/Quick Setup/I just want to start coding > > (aka i just want to serve 5TB) "I just want to start coding" looks weird in the TOC.... https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode15 go/README.md:15: If you are on Linux you can run the [quicksetup script](quicksetup.sh) like so; On 2016/06/17 17:23:31, Eric Burnett wrote: > ';' -> ':' Done. https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode17 go/README.md:17: ```shell On 2016/06/17 09:39:21, tandrii(chromium) wrote: > On 2016/06/17 09:28:44, tandrii(chromium) wrote: > > This works fine for github flavor of MD, but I'm not sure about Gerrit. > > It does work in Gerrit too: > https://chromium.googlesource.com/infra/infra/+/3b59c550ed9d6a1e73c9451bad5e8... > (I've uploaded your CL to gerrit with "git cl upload --gerrit" > https://chromium-review.googlesource.com/c/353571/) I can't actually see how you tell that it works in that interface? https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode17 go/README.md:17: ```shell On 2016/06/17 09:28:44, tandrii(chromium) wrote: > This works fine for github flavor of MD, but I'm not sure about Gerrit. Acknowledged. https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode19 go/README.md:19: wget -O- "https://chromium.googlesource.com/infra/infra/+/master/go/quicksetup.sh?format=TEXT" | bash On 2016/06/17 17:38:46, Vadim Sh. wrote: > format=TEXT actually returns base64 encoded text :-/ > > e.g. > https://chromium.googlesource.com/infra/infra/+/master/go/check.py?format=TEXT > > (that's how Gerrit works around XSS possibility). Luckily `base64` command exists. Added to the command line. https://codereview.chromium.org/2071243002/diff/1/go/README.md#newcode20 go/README.md:20: ``` On 2016/06/17 17:23:31, Eric Burnett wrote: > Can you also add a comment explaining what this will do? Something like: > > "This will create a self-contained cr-infra-go-area directory and populate it > will all necessary tools and source for using or contributing to Chromium's Go > Infrastructure. Once run, look in cr-infra-go-area/infra/go/src for the editable > source code." Done. https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh File go/quicksetup.sh (right): https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode17 go/quicksetup.sh:17: "$PWD/depot_tools/fetch" infra On 2016/06/17 09:37:26, tandrii(chromium) wrote: > I've just tried it with existing and dirty checkout of depot_tools already in > Path. Your script hangs ~2 minutes because `fetch` command runs `gclient` not > form the same depot_tools as `fetch` is, but from whatever is on $PATH. So, how > about exporting new $PATH prior to running fetch? > > export PATH="$PWD/depot_tools:$PATH" > fetch infra Done. https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode33 go/quicksetup.sh:33: export PS1="[cr-infra-go-area] \$PS1" On 2016/06/17 17:38:46, Vadim Sh. wrote: > we should add it to go/env.py instead, probably, not everyone will be using > quicksetup.sh I'm unsure the correct way to add this to go/env.py I've added it to bootstrap.py in `def get_go_environ(` but unsure if that is correct... https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode33 go/quicksetup.sh:33: export PS1="[cr-infra-go-area] \$PS1" On 2016/06/17 09:37:26, tandrii(chromium) wrote: > very nice! Acknowledged. https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode46 go/quicksetup.sh:46: # Output usage instructions On 2016/06/17 09:37:26, tandrii(chromium) wrote: > add extra echo here, so that instruction of what to run next stands out from > lots of output above. Done. https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode48 go/quicksetup.sh:48: ln -sf $ENTER_SCRIPT ~/bin/cr-infra-go-area-enter On 2016/06/17 17:38:46, Vadim Sh. wrote: > On 2016/06/17 17:23:31, Eric Burnett wrote: > > Up until here the created environment was self-contained, in that it had no > > persistent impacts for anything outside cr-infra-go-area. I don't like the > idea > > of adding things to the users' bin directory, as that breaks the containment > > (cannot be undone by just rm -rf cr-infra-go-area; installing again in a > > different path will collide on the symlink). > > > > Can you stop at printing $ENTER_SCRIPT, and leave it to the user to symlink it > > for their own benefit under whatever name they like? > > +1 I've added a "DO NOT MODIFY" header and changed it to ask about adding the script to ~/bin Is that okay? https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode48 go/quicksetup.sh:48: ln -sf $ENTER_SCRIPT ~/bin/cr-infra-go-area-enter On 2016/06/17 09:37:25, tandrii(chromium) wrote: > can we bail instead of overwriting existing file if any? > Alternatively maybe write at the top of the script (~line 23) "DO NOT MODIFY, > automatically generated, will be overwritten" Acknowledged. https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode48 go/quicksetup.sh:48: ln -sf $ENTER_SCRIPT ~/bin/cr-infra-go-area-enter On 2016/06/17 17:23:31, Eric Burnett wrote: > Up until here the created environment was self-contained, in that it had no > persistent impacts for anything outside cr-infra-go-area. I don't like the idea > of adding things to the users' bin directory, as that breaks the containment > (cannot be undone by just rm -rf cr-infra-go-area; installing again in a > different path will collide on the symlink). > > Can you stop at printing $ENTER_SCRIPT, and leave it to the user to symlink it > for their own benefit under whatever name they like? Acknowledged. https://codereview.chromium.org/2071243002/diff/1/go/quicksetup.sh#newcode54 go/quicksetup.sh:54: echo "Enter the environment by running '$PWD/enter-env.sh'" On 2016/06/17 17:23:31, Eric Burnett wrote: > Reference $ENTER_SCRIPT here for consistency? Done.
https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py File go/bootstrap.py (right): https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py#newcode436 go/bootstrap.py:436: if not prompt.startswith('[cr-infra-go-area] '): nit: '[cr-infra-go-area]' not in prompt (I can imagine there can be other PS1 modifications on top of this one). https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py#newcode437 go/bootstrap.py:437: env['PS1'] = '[cr-infra-go-area] ' + prompt good enough, though I'm very tempted to bikeshed and ask to rename it to just [go env] to save space... It is okay?)
https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py File go/bootstrap.py (right): https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py#newcode436 go/bootstrap.py:436: if not prompt.startswith('[cr-infra-go-area] '): On 2016/06/18 16:12:09, Vadim Sh. wrote: > nit: '[cr-infra-go-area]' not in prompt > > (I can imagine there can be other PS1 modifications on top of this one). Done. https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py#newcode437 go/bootstrap.py:437: env['PS1'] = '[cr-infra-go-area] ' + prompt On 2016/06/18 16:12:09, Vadim Sh. wrote: > good enough, though I'm very tempted to bikeshed and ask to rename it to just > [go env] to save space... It is okay?) I'd like the prompt to match the script you run... We could make it a command line flag?
https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py File go/bootstrap.py (right): https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py#newcode437 go/bootstrap.py:437: env['PS1'] = '[cr-infra-go-area] ' + prompt On 2016/06/20 14:19:26, mithro wrote: > On 2016/06/18 16:12:09, Vadim Sh. wrote: > > good enough, though I'm very tempted to bikeshed and ask to rename it to just > > [go env] to save space... It is okay?) > > I'd like the prompt to match the script you run... > > We could make it a command line flag? Command line flag sounds hard - users don't generally run this program directly (it's run via env.py, which is run via quickstart), so you'd have to pass it through all of them, no? Also, if a user hasn't run the quickstart script, does 'cr-infra-go-area' make any sense? I think this is the first place they'd see that, so it wouldn't be super meaningful for the prompt in the first place. So I'd vote for something small and clear in context -- [go env] or [cr go env]. (I'm also somewhat against prompt changes as a whole, speaking as someone with a complicated prompt already that would look stupid with this prefixed onto it. But I'm the kind of person who will manually export the right variables printed by env.py instead of going through the scripts each time, so my opinion may not matter here :) ). https://codereview.chromium.org/2071243002/diff/20001/go/quicksetup.sh File go/quicksetup.sh (right): https://codereview.chromium.org/2071243002/diff/20001/go/quicksetup.sh#newcode54 go/quicksetup.sh:54: read -p "Link the enter script to $BINPATH? " LINKBIN the end of the prompt should have "(y|N)" I think.
https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py File go/bootstrap.py (right): https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py#newcode437 go/bootstrap.py:437: env['PS1'] = '[cr-infra-go-area] ' + prompt On 2016/06/20 14:32:09, Eric Burnett wrote: > On 2016/06/20 14:19:26, mithro wrote: > > On 2016/06/18 16:12:09, Vadim Sh. wrote: > > > good enough, though I'm very tempted to bikeshed and ask to rename it to > just > > > [go env] to save space... It is okay?) > > > > I'd like the prompt to match the script you run... > > > > We could make it a command line flag? > > Command line flag sounds hard - users don't generally run this program directly > (it's run via env.py, which is run via quickstart), so you'd have to pass it > through all of them, no? > > Also, if a user hasn't run the quickstart script, does 'cr-infra-go-area' make > any sense? I think this is the first place they'd see that, so it wouldn't be > super meaningful for the prompt in the first place. So I'd vote for something > small and clear in context -- [go env] or [cr go env]. > > (I'm also somewhat against prompt changes as a whole, speaking as someone with a > complicated prompt already that would look stupid with this prefixed onto it. > But I'm the kind of person who will manually export the right variables printed > by env.py instead of going through the scripts each time, so my opinion may not > matter here :) ). If I make it a command line argument I can set it to something shorter by default and then have the quickstart script override it to the longer name....
lgtm with nits https://codereview.chromium.org/2071243002/diff/20001/go/README.md File go/README.md (right): https://codereview.chromium.org/2071243002/diff/20001/go/README.md#newcode9 go/README.md:9: 1) [Install depot_tools](https://www.chromium.org/developers/how-tos/install-depot-tools) Use google markdown style https://github.com/google/styleguide/blob/gh-pages/docguide/style.md https://codereview.chromium.org/2071243002/diff/20001/go/README.md#newcode26 go/README.md:26: Style nit; put 2 empty lines above each title. https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py File go/bootstrap.py (right): https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py#newcode437 go/bootstrap.py:437: env['PS1'] = '[cr-infra-go-area] ' + prompt On 2016/06/21 06:55:05, mithro wrote: > If I make it a command line argument I can set it to something shorter by > default and then have the quickstart script override it to the longer name.... My bikeshed color would be '\033[33;1mcit\033[0m ' + prompt but I don't mind much. https://codereview.chromium.org/2071243002/diff/20001/go/quicksetup.sh File go/quicksetup.sh (right): https://codereview.chromium.org/2071243002/diff/20001/go/quicksetup.sh#newcode6 go/quicksetup.sh:6: set -e set -eu https://codereview.chromium.org/2071243002/diff/20001/go/quicksetup.sh#newcode44 go/quicksetup.sh:44: chmod a+x $ENTER_SCRIPT add two spaces after EOF, otherwise it makes scanning this file harder.
The CQ bit was checked by tansell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2071243002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2071243002/diff/20001/go/README.md File go/README.md (right): https://codereview.chromium.org/2071243002/diff/20001/go/README.md#newcode9 go/README.md:9: 1) [Install depot_tools](https://www.chromium.org/developers/how-tos/install-depot-tools) On 2016/06/21 12:21:44, M-A Ruel wrote: > Use google markdown style > https://github.com/google/styleguide/blob/gh-pages/docguide/style.md I assume you mean the "replace ) with ."? I couldn't find any other violation of the style guide. If so - done. https://codereview.chromium.org/2071243002/diff/20001/go/README.md#newcode26 go/README.md:26: On 2016/06/21 12:21:44, M-A Ruel wrote: > Style nit; put 2 empty lines above each title. Done. https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py File go/bootstrap.py (right): https://codereview.chromium.org/2071243002/diff/20001/go/bootstrap.py#newcode437 go/bootstrap.py:437: env['PS1'] = '[cr-infra-go-area] ' + prompt On 2016/06/21 12:21:44, M-A Ruel wrote: > On 2016/06/21 06:55:05, mithro wrote: > > If I make it a command line argument I can set it to something shorter by > > default and then have the quickstart script override it to the longer name.... > > My bikeshed color would be '\033[33;1mcit\033[0m ' + prompt but I don't mind > much. Adding a command line flag turned out to be a bigger change than I wanted to go with, so I used the INFRA_PROMPT_TAG environment variable. The default value is the short "[cr go] ". If you want nothing, "export INFRA_PROMPT_TAG=''" The quicksetup script uses "[cr-infra-go-env] " to match the enter script's name. https://codereview.chromium.org/2071243002/diff/20001/go/quicksetup.sh File go/quicksetup.sh (right): https://codereview.chromium.org/2071243002/diff/20001/go/quicksetup.sh#newcode6 go/quicksetup.sh:6: set -e On 2016/06/21 12:21:44, M-A Ruel wrote: > set -eu Done. https://codereview.chromium.org/2071243002/diff/20001/go/quicksetup.sh#newcode44 go/quicksetup.sh:44: chmod a+x $ENTER_SCRIPT On 2016/06/21 12:21:44, M-A Ruel wrote: > add two spaces after EOF, otherwise it makes scanning this file harder. I assume you mean newlines? Done. https://codereview.chromium.org/2071243002/diff/20001/go/quicksetup.sh#newcode54 go/quicksetup.sh:54: read -p "Link the enter script to $BINPATH? " LINKBIN On 2016/06/20 14:32:09, Eric Burnett wrote: > the end of the prompt should have "(y|N)" I think. Done.
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by tansell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@chromium.org, maruel@chromium.org Link to the patchset: https://codereview.chromium.org/2071243002/#ps100001 (title: "cd into the source area.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2071243002/100001
https://codereview.chromium.org/2071243002/diff/20001/go/quicksetup.sh File go/quicksetup.sh (right): https://codereview.chromium.org/2071243002/diff/20001/go/quicksetup.sh#newcode44 go/quicksetup.sh:44: chmod a+x $ENTER_SCRIPT On 2016/06/22 04:39:20, mithro wrote: > On 2016/06/21 12:21:44, M-A Ruel wrote: > > add two spaces after EOF, otherwise it makes scanning this file harder. > > I assume you mean newlines? > > Done. Err yes :)
Message was sent while issue was closed.
Description was changed from ========== infra/go: Adding setup instructions and quick setup script. The setup instructions explain how to get the "Chromium Infra Go Area" and the quick setup script gets you started with one command and provides a wrapper. This was split out of https://codereview.chromium.org/1807463005/ ========== to ========== infra/go: Adding setup instructions and quick setup script. The setup instructions explain how to get the "Chromium Infra Go Area" and the quick setup script gets you started with one command and provides a wrapper. This was split out of https://codereview.chromium.org/1807463005/ Committed: https://chromium.googlesource.com/infra/infra/+/e8441f571e7a2198e7837bb03b147... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/infra/infra/+/e8441f571e7a2198e7837bb03b147...
Message was sent while issue was closed.
On 2016/06/22 12:48:46, commit-bot: I haz the power wrote: > Committed patchset #5 (id:100001) as > https://chromium.googlesource.com/infra/infra/+/e8441f571e7a2198e7837bb03b147... 1) Not sure why but the landed commit https://chromium.googlesource.com/infra/infra/+/e8441f571e7a2198e7837bb03b147... points to a different CL 2) this broke my dynamic prompt: it used to show current git branch, but now when I run eval `~/cr/infra/infra/go/env.py` it remembers the git branch that was active at that time and never updates it when i switch to a different branch. please fix
Message was sent while issue was closed.
On 2016/06/23 02:50:09, nodir1 wrote: > On 2016/06/22 12:48:46, commit-bot: I haz the power wrote: > > Committed patchset #5 (id:100001) as > > > https://chromium.googlesource.com/infra/infra/+/e8441f571e7a2198e7837bb03b147... > > 1) Not sure why but the landed commit > https://chromium.googlesource.com/infra/infra/+/e8441f571e7a2198e7837bb03b147... > points to a different CL > 2) this broke my dynamic prompt: it used to show current git branch, but now > when I run > eval `~/cr/infra/infra/go/env.py` > it remembers the git branch that was active at that time and never updates it > when i switch to a different branch. please fix $ echo $PS1 \[\033[1;31m\]\W $ \[\033[37m\]`git branch 2> /dev/null | grep -e ^* | sed -E s/^\\\\\*\ \(.+\)$/\(\\\\\1\)\ /`\[\033[0m\]
Message was sent while issue was closed.
On 2016/06/23 02:56:30, nodir1 wrote: > On 2016/06/23 02:50:09, nodir1 wrote: > > On 2016/06/22 12:48:46, commit-bot: I haz the power wrote: > > > Committed patchset #5 (id:100001) as > > > > > > https://chromium.googlesource.com/infra/infra/+/e8441f571e7a2198e7837bb03b147... > > > > 1) Not sure why but the landed commit > > > https://chromium.googlesource.com/infra/infra/+/e8441f571e7a2198e7837bb03b147... > > points to a different CL > > 2) this broke my dynamic prompt: it used to show current git branch, but now > > when I run > > eval `~/cr/infra/infra/go/env.py` > > it remembers the git branch that was active at that time and never updates it > > when i switch to a different branch. please fix > > $ echo $PS1 > \[\033[1;31m\]\W $ \[\033[37m\]`git branch 2> /dev/null | grep -e ^* | sed -E > s/^\\\\\*\ \(.+\)$/\(\\\\\1\)\ /`\[\033[0m\] I see what is happening. The "eval ``" is running the commands in your prompt. This will actually happen with any output of env.py but I guess nothing else has this stuff in it. Temporary work around is to disable this with a "export INFRA_PROMPT_TAG=''" Will send out a fix ASAP.
Message was sent while issue was closed.
dnj@google.com changed reviewers: + dnj@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/2071243002/diff/100001/go/bootstrap.py File go/bootstrap.py (right): https://codereview.chromium.org/2071243002/diff/100001/go/bootstrap.py#newcod... go/bootstrap.py:433: # Add a tag to the prompt This broke my shell prompt. Is it really necessary? It's presumptuous and a bit invasive, and I'd prefer to not have to add an opt-out variable to my environment just for this.
Message was sent while issue was closed.
On 2016/07/01 01:40:42, dnj (Google) wrote: > https://codereview.chromium.org/2071243002/diff/100001/go/bootstrap.py > File go/bootstrap.py (right): > > https://codereview.chromium.org/2071243002/diff/100001/go/bootstrap.py#newcod... > go/bootstrap.py:433: # Add a tag to the prompt > This broke my shell prompt. Is it really necessary? It's presumptuous and a bit > invasive, and I'd prefer to not have to add an opt-out variable to my > environment just for this. How did it end up breaking your prompt? Modifying the prompt this way is very common for virtual environment like tools, pretty much all of them do this. (Examples; Python's virtualenv, Ruby's rvm, conda)
Message was sent while issue was closed.
> How did it end up breaking your prompt? Modifying the prompt this way is very > common for virtual environment like tools, pretty much all of them do this. > (Examples; Python's virtualenv, Ruby's rvm, conda) See: https://codereview.chromium.org/2121653002/ I understand that other utilities do this, but it's also notable that not every shell-modifying utility does modify the prompt. If they did, everyone's prompt would become a mess of tags. Virtualenv, for example, addresses this by letting you easily configure how (or if) it modifies your prompt. The presumption that the tag "[cr go]" looks good at the beginning of every prompt is not correct. Minimalist prompts are a good example of this. In this case, the problem with my prompt was that the shell export logic wasn't sufficient to handle my prompt string. I also execute "env.py" as part of a larger environment script that already augments my shell with a context, so "[cr go]" is redundant for me. This is why I said that it was presumptuous.
Message was sent while issue was closed.
> How did it end up breaking your prompt? Modifying the prompt this way is very > common for virtual environment like tools, pretty much all of them do this. > (Examples; Python's virtualenv, Ruby's rvm, conda) See: https://codereview.chromium.org/2121653002/ I understand that other utilities do this, but it's also notable that not every shell-modifying utility does modify the prompt. If they did, everyone's prompt would become a mess of tags. Virtualenv, for example, addresses this by letting you easily configure how (or if) it modifies your prompt. The presumption that the tag "[cr go]" looks good at the beginning of every prompt is not correct. Minimalist prompts are a good example of this. In this case, the problem with my prompt was that the shell export logic wasn't sufficient to handle my prompt string. I also execute "env.py" as part of a larger environment script that already augments my shell with a context, so "[cr go]" is redundant for me. This is why I said that it was presumptuous.
Message was sent while issue was closed.
Oops hit the button twice. Sorry about that. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
