Chromium Code Reviews
Help | Chromium Project | Sign in
(38)

Issue 222183002: Call install-build-deps.sh from install-build-deps-android.sh (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year ago by Primiano Tucci
Modified:
1 year ago
CC:
chromium-reviews, himanshu.mistri
Visibility:
Public.

Description

Call install-build-deps.sh from install-build-deps-android.sh install-build-deps-android.sh relies on the assumption that install-build-deps.sh has been previously invoked. At this point, is makes sense for the Android script itself invoking its parent dependency. See chromium-dev discussion: http://goo.gl/dKDX3R BUG= NOTRY=true Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261535

Patch Set 1 #

Patch Set 2 : Actually, just invoke the full linux deps script #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -5 lines) Patch
M build/install-build-deps-android.sh View 1 1 chunk +3 lines, -5 lines 4 comments Download
Commit: CQ not working?

Messages

Total messages: 14 (0 generated)
Primiano Tucci
1 year ago (2014-04-02 13:41:51 UTC) #1
Torne
This shouldn't be necessary, no? It's already in install-build-deps.sh and I don't think it's expected ...
1 year ago (2014-04-02 13:45:43 UTC) #2
himanshu.mistri
On 2014/04/02 13:45:43, Torne wrote: > This shouldn't be necessary, no? It's already in install-build-deps.sh ...
1 year ago (2014-04-02 14:06:27 UTC) #3
Primiano Tucci
Right, Torne and I just had a discussion offline. Uploaded a 2nd patchset to just ...
1 year ago (2014-04-02 14:09:25 UTC) #4
Primiano Tucci
Nico, would you mind taking a look to this 2-lines CL? It is to strive ...
1 year ago (2014-04-03 20:15:50 UTC) #5
Nico (afk until Mon Apr 20)
lgtm, makes sense, thanks! https://codereview.chromium.org/222183002/diff/20001/build/install-build-deps-android.sh File build/install-build-deps-android.sh (right): https://codereview.chromium.org/222183002/diff/20001/build/install-build-deps-android.sh#newcode21 build/install-build-deps-android.sh:21: "$(dirname "${BASH_SOURCE[0]}")/install-build-deps.sh" \ Is `$(dirname ...
1 year ago (2014-04-03 20:19:56 UTC) #6
Nico (afk until Mon Apr 20)
(maybe reference the chromium-dev thread somewhere in the cl description)
1 year ago (2014-04-03 20:20:14 UTC) #7
Primiano Tucci
https://codereview.chromium.org/222183002/diff/20001/build/install-build-deps-android.sh File build/install-build-deps-android.sh (right): https://codereview.chromium.org/222183002/diff/20001/build/install-build-deps-android.sh#newcode21 build/install-build-deps-android.sh:21: "$(dirname "${BASH_SOURCE[0]}")/install-build-deps.sh" \ On 2014/04/03 20:19:56, Nico wrote: > ...
1 year ago (2014-04-03 20:54:10 UTC) #8
Nico (afk until Mon Apr 20)
https://codereview.chromium.org/222183002/diff/20001/build/install-build-deps-android.sh File build/install-build-deps-android.sh (right): https://codereview.chromium.org/222183002/diff/20001/build/install-build-deps-android.sh#newcode21 build/install-build-deps-android.sh:21: "$(dirname "${BASH_SOURCE[0]}")/install-build-deps.sh" \ On 2014/04/03 20:54:10, Primiano Tucci wrote: ...
1 year ago (2014-04-03 20:58:36 UTC) #9
Primiano Tucci
https://codereview.chromium.org/222183002/diff/20001/build/install-build-deps-android.sh File build/install-build-deps-android.sh (right): https://codereview.chromium.org/222183002/diff/20001/build/install-build-deps-android.sh#newcode21 build/install-build-deps-android.sh:21: "$(dirname "${BASH_SOURCE[0]}")/install-build-deps.sh" \ On 2014/04/03 20:58:36, Nico wrote: > ...
1 year ago (2014-04-03 21:30:31 UTC) #10
Primiano Tucci
The CQ bit was checked by primiano@chromium.org
1 year ago (2014-04-03 21:34:35 UTC) #11
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/primiano@chromium.org/222183002/20001
1 year ago (2014-04-03 21:36:33 UTC) #12
I haz the power (commit-bot)
Change committed as 261535
1 year ago (2014-04-03 21:40:20 UTC) #13
Nico (afk until Mon Apr 20)
1 year ago (2014-04-03 21:42:53 UTC) #14
Message was sent while issue was closed.
On 2014/04/03 21:30:31, Primiano Tucci wrote:
>
https://codereview.chromium.org/222183002/diff/20001/build/install-build-deps...
> File build/install-build-deps-android.sh (right):
> 
>
https://codereview.chromium.org/222183002/diff/20001/build/install-build-deps...
> build/install-build-deps-android.sh:21: "$(dirname
> "${BASH_SOURCE[0]}")/install-build-deps.sh" \
> On 2014/04/03 20:58:36, Nico wrote:
> > On 2014/04/03 20:54:10, Primiano Tucci wrote:
> > > On 2014/04/03 20:19:56, Nico wrote:
> > > > Is `$(dirname "${0}") enough? It's not like there's several levels of
> > sourcing
> > > > here.
> > > 
> > > I think the difference between $0 vs $BASH_SOURCE is that $BASH_SOURCE is
> > safer,
> > > in the sense that it should behave like __file__in python.
> > > Essentially, I fear that if you run the script trhough ".
> > install-build-deps.sh"
> > > oppposite to "./install... or ./build/install...." that might fail.
> > 
> > We use $0 in a bunch of places, and I never heard of problems like this.
> 
> ~ $ cat foo
> #!/bin/bash
> echo My dir-1 is $(dirname "${BASH_SOURCE[0]}")/
> echo My dir-2 is $(dirname "$0")/
> 
> /tmp $ ./foo
> My dir-1 is ./
> My dir-2 is ./
> 
> /tmp $ . foo
> My dir-1 is ./
> My dir-2 is /bin/ #Ha wrong
> 
> /tmp $ /tmp/foo
> My dir-1 is /tmp/
> My dir-2 is /tmp/
> 
> /tmp $ . /tmp/foo
> My dir-1 is /tmp/
> My dir-2 is /bin/  # Ha wrong again!!!
> /tmp $ 
> 
> That's it! I think just nobody complained, but BASH_SOURCE is IMHO actually
more
> reliable.

No, that's exactly my point: This script isn't intended to be sourced.

> The real question is why we keep writing bash scripts, but that's another
story
> :)

Agreed, but the Real Evil are bash scripts that have to be sourced instead of
run (runnable bash scripts can be replaced with python once they get longer).
BASH_SOURCE is only useful for sourceable scripts from what I understand.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 700cc9d