Merge lp:~pundiramit/linaro-android-build-tools/access_private_manifests into lp:linaro-android-build-tools

Proposed by Amit Pundir on 2013-01-23
Status: Merged
Merged at revision: 554
Proposed branch: lp:~pundiramit/linaro-android-build-tools/access_private_manifests
Merge into: lp:linaro-android-build-tools
Diff against target: 157 lines (+46/-18)
2 files modified
build-scripts/build-android (+6/-0)
build-scripts/create-user-build-script (+40/-18)
To merge this branch: bzr merge lp:~pundiramit/linaro-android-build-tools/access_private_manifests
Reviewer Review Type Date Requested Status
Данило Шеган (community) 2013-01-23 Approve on 2013-01-28
Review via email:

Description of the change

Prompt for login/access-id when user try to download private manifests.

To post a comment you must log in.
Данило Шеган (danilo) wrote :

Does it not make sense to read ID from some environment variable as well? (so people can simply set it in their .bashrc or .profile to ensure the same ID is used going forward)

Of course, if you make it an environment variable, I suppose naming it like LINARO_ANDROID_ACCESS_ID would be better than just "ID".

Btw, what are the reasons to replace ".*-bot" with ${ID}, and not simply replace "default-bot"?

review: Needs Fixing
Amit Pundir (pundiramit) wrote :

I did try to read userID from .bashrc/.profile/.gitconfig/bazaar.conf at first but the thing is that this userID is generated by linaro-private git administration and it may not be consistent with a user's existing usernames. At least in my case, the admin gave me an username "amitpundir" whereas my launchpad/git/gerrit/bzr ID is "pundiramit".

I can certainly rename ID to a much understandable LINARO_ANDROID_ACCESS_ID environmental variable.

I did not understand the last concern.
I'm initializing ID with "default-bot", which is a default unauthorised bot and do not have access to any private gits. I can very well leave ID uninitialized as well.
Once user provide correct access ID, I override "default-bot" with user provided ID and then replace "*-bot" e.g. "linaro-big-little-switcher-bot" with ID. I used "*-bot" because it could be any project specific bot. I'm counting on future default bots to be named on $PROJECT-bot as well.

Данило Шеган (danilo) wrote :

I think it's fine not to try to guess the user ID. What I meant was that it should be possible for someone to simply do something like:

  export LINARO_ANDROID_ACCESS_ID=my-linaro-username

If you unconditionally set the variable in the script, that will not be possible. You can do that by setting the variable in the script as (hyphen to separate the variable name and default value):

  VARNAME=${VARNAME-default value}



Then, someone would be able to put "export LINARO_ANDROID_ACCESS_ID=danilo" in their .bashrc and not worry about it when using "./".

555. By Amit Pundir on 2013-01-23

user build script: check for already exported access ID

Amit Pundir (pundiramit) wrote :

Script fixed to check for already exported LINARO_ANDROID_ACCESS_ID. An exported LINARO_ANDROID_ACCESS_ID can be override with "./ -l <access-id>" switch though.

Данило Шеган (danilo) wrote :

This looks good, did you perhaps test it out (a link to a test build would be nice) to confirm escaping works as expected (it's all too easy to make a typo somewhere, and a best test is to get a runnable script out :).

review: Approve
Amit Pundir (pundiramit) wrote :

I did test the changes separately on a local script. The best way, indeed, is to get a runnable script out and test it to confirm that escaping work as expected.

556. By Amit Pundir on 2013-01-24

user build script: make SOURCE_OVERLAY optional

557. By Amit Pundir on 2013-01-24

user build script: invoke apt-get install only for missing packages

Amit Pundir (pundiramit) wrote :

Pushed a couple of more/unrelated changes to this branch:
1) Make source_overlay optional.
2) Invoke apt-get, only for missing packages. This fixes an issue on hackbox where "apt-get install" returns exit code 1, when trying to install already installed packages.

Данило Шеган (danilo) wrote :

The changes for LINARO_ANDROID_ACCESS_ID look good.

However, as I said in the previous MP at

I don't want us to allow builds on without the overlay. How does this ensure this still remains the case?

review: Needs Information
558. By Amit Pundir on 2013-01-24

user build script: protect source overlay usage with a flag

Amit Pundir (pundiramit) wrote :

Ok. With this new patchset, I changed how I was handling SOURCE_OVERLAY variable. Since SOURCE_OVERLAY variable is something which may or may not be directly used by jenkins for build purpose, I'm not playing around it anymore.

What I did this time is that I introduced a new flag SOURCE_OVERLAY_OPTIONAL which is set to "1" to make overlay optional. This SOURCE_OVERLAY_OPTIONAL flag will be toggled to "0" when we explicitly set overlay using "-o <overlay>" switch. To protect S_OVERLAY portion of the script, I have put relevant conditional checks for S_O_OPTIONAL flag.

Let me know if you have any concerns on this one.

Данило Шеган (danilo) wrote :

This generally looks good. I think only one thing is missing: what happens when someone puts the SOURCE_OVERLAY_OPTIONAL in the build configuration on android-build?

Since the config file is generated on the build with the script

based on the data passed in from build config, we should make sure that the SOURCE_OVERLAY_OPTIONAL triggers an error there.

559. By Amit Pundir on 2013-01-25

build android: make sure no one sets SOURCE_OVERLAY_OPTIONAL in official build configurations

Amit Pundir (pundiramit) wrote :

So I figured out that even this script "build-scripts/build-android" can be modified to make sure that no one put SOURCE_OVERLAY_OPTIONAL in the official build configurations. Is my understanding correct?

$ bzr diff
=== modified file 'build-scripts/build-android'
--- build-scripts/build-android 2012-12-19 10:05:03 +0000
+++ build-scripts/build-android 2013-01-25 11:29:39 +0000
@@ -14,6 +14,12 @@
     exit 1

+ echo "ERROR: SOURCE_OVERLAY_OPTIONAL should not be set in official build configuration."
+ echo " It is meant to be set only in local build scripts to bypass overlays."
+ exit 1
 source "${BUILD_SCRIPT_ROOT}"/helpers

 trap infrastructure_error ERR


Данило Шеган (danilo) wrote :

Yeah, this looks good. I am testing your branch now with let's see if it works as expected. If it does, I'll merge it in.

Данило Шеган (danilo) wrote :

Ok, I tested a few cases (based on galaxynexus-linaro build):


    Ok, fails with SOURCE_OVERLAY missing.


    OK, fails with your new error message.

 3. SOURCE_OVERLAY present, no SOURCE_OVERLAY_OPTIONAL (regular builds)

    Just starting now. If that one builds ok, I'll merge your branch asap.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'build-scripts/build-android'
2--- build-scripts/build-android 2012-12-19 10:05:03 +0000
3+++ build-scripts/build-android 2013-01-25 11:33:23 +0000
4@@ -14,6 +14,12 @@
5 exit 1
6 fi
9+ echo "ERROR: SOURCE_OVERLAY_OPTIONAL should not be set in official build configuration."
10+ echo " It is meant to be set only in local build scripts to bypass overlays."
11+ exit 1
14 source "${BUILD_SCRIPT_ROOT}"/helpers
16 trap infrastructure_error ERR
18=== modified file 'build-scripts/create-user-build-script'
19--- build-scripts/create-user-build-script 2013-01-23 09:48:37 +0000
20+++ build-scripts/create-user-build-script 2013-01-25 11:33:23 +0000
21@@ -17,7 +17,7 @@
22 USAGE_SUM="'Usage: \$0 -m <manifest.xml> -o <overlay.tar> [ -t -d directory -l login ]'"
23 USAGE_OVERLAY="'\\n -m <manifest> If -t is not used, then using a browser with cookies you\\n must download the pinned manifest from:\\n $PINNED_MANIFEST_URL\\n -o The path to the vendor required overlay.\\n Can be downloaded from$SOURCE_OVERLAY\\n'"
27 else
28 USAGE_SUM="'Usage: \$0 [ -t -d directory -l login ]'"
29 fi
30@@ -32,7 +32,10 @@
32 EXACT=1
33 DIR=android
35+if [ -z "\${LINARO_ANDROID_ACCESS_ID}" ] ; then
40 usage()
41 {
42@@ -44,7 +47,7 @@
43 echo " Default: \${DIR}"
44 echo " -l <login-id> login-id to clone from linaro-private git repositories"
45 echo " If in doubt, kindly raise access request to"
46- echo " Default: \${ID}"
47+ echo " Default: \${LINARO_ANDROID_ACCESS_ID}"
48 exit 1
49 }
51@@ -52,7 +55,7 @@
52 case \$optn in
54 d ) DIR=\$OPTARG;;
55- l ) ID=\$OPTARG;;
57 t ) EXACT=0;;
58 h ) usage; exit 1;;
59 esac
60@@ -77,16 +80,14 @@
61 fi
63 echo "Checking and installing missing dependencies if any .. .."
64-sudo apt-get install \${PKGS}
66-MISSING=\`dpkg-query -W -f='\${Status}\n' \${PKGS} 2>&1 | grep 'No packages found matching' | cut -d' ' -f5\`
67+MISSING=\`dpkg-query -W -f='\${Status}\n' \${PKGS} 2>&1 | grep -i 'No packages found matching' | cut -d' ' -f6\`
68 if [ -n "\$MISSING" ] ; then
69- echo "Missing required packages:"
70+ echo -n "Missing required packages: "
71 for m in \$MISSING ; do
72 echo -n "\${m%?} "
73 done
74 echo
75- exit 1
76+ sudo apt-get install \${MISSING}
77 fi
79 EOF
80@@ -96,9 +97,11 @@
81 echo "ERROR: no pinned manifest provided. Please download from $PINNED_MANIFEST_URL. This must be done from a browser that accepts cookies."
82 exit 1
83 fi
84-if [ "a\$SOURCE_OVERLAY" == "a" ]; then
85- echo "ERROR: no source overlay provided. Please download from$SOURCE_OVERLAY. This must be done from a browser that accepts cookies."
86- exit 1
87+if [ \$SOURCE_OVERLAY_OPTIONAL -ne 1 ]; then
88+ if [ "a\$SOURCE_OVERLAY" == "a" ]; then
89+ echo "ERROR: no source overlay provided. Please download from$SOURCE_OVERLAY. This must be done from a browser that accepts cookies."
90+ exit 1
91+ fi
92 fi
93 EOF
94 fi
95@@ -118,7 +121,24 @@
96 jenkins_configs_method()
97 {
98 cat <<EOF
100+# check for linaro private manifests
101+PM=\`echo ${MANIFEST_REPO} | grep -i "linaro-private" | wc -l\`
102+if [ \${PM} -gt 0 ] ; then
103+ if [ "\${LINARO_ANDROID_ACCESS_ID}" == "default-bot" ] ; then
104+ echo "You must specify valid login/access-id to clone from linaro-private manifest repositories."
105+ echo "Press "y" to continue (which may result in incomplete build or failure), OR"
106+ echo "Press "n" to enter login details, OR"
107+ echo "Press "h" for help."
108+ read NEXT
109+ if [ \${NEXT} == n ] ; then
110+ echo "Enter login/access-id:"
112+ elif [ \${NEXT} == h ] ; then
113+ usage
114+ fi
115+ fi
117+export MANIFEST_REPO=`echo ${MANIFEST_REPO} | sed 's/\/\/.*-bot@/\/\/'"\${LINARO_ANDROID_ACCESS_ID}"'@/'`
121@@ -182,7 +202,7 @@
122 # check for linaro private git repositories
123 PRI=\`grep -i "linaro-private" .repo/manifests/\${MANIFEST_FILENAME} | wc -l\`
124 if [ \${PRI} -gt 0 ] ; then
125- if [ "\${ID}" == "default-bot" ] ; then
126+ if [ "\${LINARO_ANDROID_ACCESS_ID}" == "default-bot" ] ; then
127 echo "You must specify valid login/access-id to clone from linaro-private git repositories."
128 echo "Press "y" to continue (which may result in incomplete build), OR"
129 echo "Press "n" to enter login details, OR"
130@@ -190,12 +210,12 @@
131 read NEXT
132 if [ \${NEXT} == n ] ; then
133 echo "Enter login/access-id:"
134- read ID
136 elif [ \${NEXT} == h ] ; then
137 usage
138 fi
139 fi
140- sed -i 's/\/\/.*-bot@/\/\/'"\${ID}"'@/' .repo/manifests/\${MANIFEST_FILENAME}
141+ sed -i 's/\/\/.*-bot@/\/\/'"\${LINARO_ANDROID_ACCESS_ID}"'@/' .repo/manifests/\${MANIFEST_FILENAME}
142 fi
143 ./repo sync
145@@ -213,8 +233,10 @@
146 if [ -n "$SOURCE_OVERLAY" ]; then
147 cat <<EOF
149-# extract the vendor's source overlay
150-tar -x -a -f "\$SOURCE_OVERLAY" -C .
151+if [ \$SOURCE_OVERLAY_OPTIONAL -ne 1 ]; then
152+ # extract the vendor's source overlay
153+ tar -x -a -f "\$SOURCE_OVERLAY" -C .
155 EOF
156 fi
157 echo


People subscribed via source and target branches