Merge lp:~fginther/ubuntu-test-cases/remove-source-package into lp:ubuntu-test-cases/touch

Proposed by Francis Ginther
Status: Merged
Approved by: Francis Ginther
Approved revision: 399
Merged at revision: 401
Proposed branch: lp:~fginther/ubuntu-test-cases/remove-source-package
Merge into: lp:ubuntu-test-cases/touch
Diff against target: 128 lines (+18/-32)
5 files modified
scripts/boottest.sh (+12/-23)
tests/getinstalledpkgs/debian/changelog (+1/-1)
tests/getinstalledpkgs/debian/control (+2/-2)
tests/getinstalledpkgs/debian/tests/control (+2/-2)
tests/getinstalledpkgs/debian/tests/getinstalledpkgs.template (+1/-4)
To merge this branch: bzr merge lp:~fginther/ubuntu-test-cases/remove-source-package
Reviewer Review Type Date Requested Status
Paul Larson Approve
Evan Pending
Review via email: mp+260854@code.launchpad.net

This proposal supersedes a proposal from 2015-05-29.

Description of the change

I'm hijacking laney's MP as another change was identified through testing (this was to remove the need for the boottest.py wrapper to have the package source).

oxide-qt's boottest fails. psivaa says this is because getpkgsrc can't download the source package due to the device not having enough space.

Looking at the test script it seems as if the whole orig isn't needed - you only want the debian/ part to run the tests, and in "getpkgsrc" itself (outputs the binary packages from a source that are already installed) you can get the necessary information from apt's database directly without needing any part of the source.

We can refactor both parts

  - Make getpkgsrc use grep-aptavail from dctrl-tools to do the source to binary mapping. Then it isn't getting the source any more, so I rename it to getinstalledpkgs.
  - The boottest.py wrapper does not need the source package at all, it just needs to inject the list of to-install-binary packages generated by getinstalledpkgs into the boottest dep8 control file

Tested with oxide-qt here:
- http://d-jenkins.ubuntu-ci:8080/view/Wily/view/BootTest/job/fjg-boottest/24/

To post a comment you must log in.
Revision history for this message
Evan (ev) wrote : Posted in a previous version of this proposal

I'm not convinced --diff-only will work here.

review: Needs Information
Revision history for this message
Evan (ev) wrote : Posted in a previous version of this proposal

Having getpkgsrc use grep-aptavail works. This block is here to calculate the binary packages generated by the source package under test so that they can be fed in as test dependencies of the DEP8 boot test.

Revision history for this message
Iain Lane (laney) wrote : Posted in a previous version of this proposal

On Fri, May 29, 2015 at 05:42:37PM -0000, Evan Dandrea wrote:
> Isn't this going to fail for native packages? Also, ${SRC_PKG_NAME} :)

Yes. Good catch (on both). Can you think of any other case? Otherwise,
we could try --diff-only and if this doesn't download any files then do
it again without.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message
Iain Lane (laney) wrote : Posted in a previous version of this proposal

On Sat, May 30, 2015 at 05:51:21PM -0000, Iain Lane wrote:
> On Fri, May 29, 2015 at 05:42:37PM -0000, Evan Dandrea wrote:
> > Isn't this going to fail for native packages? Also, ${SRC_PKG_NAME} :)
>
> Yes. Good catch (on both). Can you think of any other case? Otherwise,
> we could try --diff-only and if this doesn't download any files then do
> it again without.

Pushed something like that, with fixes for other source formats too.
Could you re-review?

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message
Francis Ginther (fginther) wrote : Posted in a previous version of this proposal

I'm currently testing this and will have some more input later. Right now, I don't see any reason why the code that runs on the host system needs to change. The fix to stop downloading the entire source package in getinstalledpkgs should be all that is needed.

Revision history for this message
Iain Lane (laney) wrote : Posted in a previous version of this proposal

On Mon, Jun 01, 2015 at 03:23:35PM -0000, Francis Ginther wrote:
> I'm currently testing this and will have some more input later. Right now, I don't see any reason why the code that runs on the host system needs to change. The fix to stop downloading the entire source package in getinstalledpkgs should be all that is needed.

I thought that adt copies the source tree over to the target system.

If it doesn't, then that is right and this becomes just a space
optimisation so we can drop it if you want.

(Actually, I don't see why the package under test's debian/ directory is
needed at all at this step - it seems to me like it could just be a
skeleton whose debian/tests/control we generate from
needs_install.packages. But I didn't know for sure so went for this
route.

Actually┬▓, I don't get the reason why we generate this list at all.
Surely the boottest infrastructure has the list of packages it cares
about because it needs to know which packages to trigger a boottest for
in the first place and this script could consult a mapping into that
list directly.

Those are asides that don't need to block this review.)

p.s. still couldn't test it so it might break in some boneheaded way;
feel free to fix up directly and then merge yourself if it's easier once
you see what's wrong

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message
Francis Ginther (fginther) wrote : Posted in a previous version of this proposal

Thanks for the response. Looking at this deeper, I have to agree with you. This section of code appears to be doing more then it needs to. The boottest is setup using the dep8 format, but it is completely independent of any dep8 test that the source package under test may have defined, but this separation appears to be confused.

From my testing so far, the changes to tests/getinstalledpkgs/* are doing the right thing. If This is the case for a few more test cases. I'll pull this set of changes as you suggest and at least fix the issue caused by large source packages.

Revision history for this message
Francis Ginther (fginther) wrote :

I've resubmitted this MP after removing the need for boottest.sh to have the source package. Now it just executes the boottest script, modifying the list of binary packages in place.

Revision history for this message
Paul Larson (pwlars) wrote :

I've tried this locally and got it to pass on the first try with using ca-certificates as a test package. It still doesn't properly update the package, but I know that's coming in another one. I think it's at least as good as what we have right now and doesn't seem to cause new problems, so I'm +1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'scripts/boottest.sh'
2--- scripts/boottest.sh 2015-03-16 16:20:32 +0000
3+++ scripts/boottest.sh 2015-06-02 16:14:14 +0000
4@@ -58,7 +58,7 @@
5 fi
6
7 # Ensure we leave a usable phone
8- [ -z ${NODE_NAME} ] || test-runner/scripts/recover.py ${NODE_NAME}
9+ [ -z ${NODE_NAME} ] || ${BASEDIR}/scripts/recover.py ${NODE_NAME}
10
11 # Leave a parting message
12 # (disable command tracing as it confuses the output)
13@@ -146,16 +146,16 @@
14 --- ${ADT_VIRT}"
15
16
17-# Inject the package name into getpkgsrc DEP8 test
18-FROM=${TESTS}/getpkgsrc/debian/tests/getpkgsrc.template
19-TARGET=${TESTS}/getpkgsrc/debian/tests/getpkgsrc
20+# Inject the package name into getinstalledpkgs DEP8 test
21+FROM=${TESTS}/getinstalledpkgs/debian/tests/getinstalledpkgs.template
22+TARGET=${TESTS}/getinstalledpkgs/debian/tests/getinstalledpkgs
23 sed -e "s/{{ source_package }}/${SRC_PKG_NAME}/" ${FROM} > ${TARGET}
24
25 # Execute a first test to get the package source tree from the testbed.
26 PKG_SRC_DIR=pkgsrc
27 rm -fr ${PKG_SRC_DIR} || true
28 set +e
29-${ADT_CMD} --unbuilt-tree ${TESTS}/getpkgsrc -o ${PKG_SRC_DIR} ${ADT_OPTS}
30+${ADT_CMD} --unbuilt-tree ${TESTS}/getinstalledpkgs -o ${PKG_SRC_DIR} ${ADT_OPTS}
31 RET=$?
32 set -e
33
34@@ -167,7 +167,7 @@
35 echo "$RELEASE $ARCH $SRC_PKG_NAME" > $errfile
36 [ -f "$errfile" ] && ${RSYNC} -a $errfile $RSYNC_DEST/${RELEASE}/tmp/ || true
37 # Ensure we leave a usable phone
38- [ -z ${NODE_NAME} ] || test-runner/scripts/recover.py ${NODE_NAME}
39+ [ -z ${NODE_NAME} ] || ${BASEDIR}/scripts/recover.py ${NODE_NAME}
40
41 exit $RET
42 fi
43@@ -179,27 +179,16 @@
44 RET=$?
45 set -e
46 else
47- SOURCE_DIR=$(ls -d ${PKG_SRC_DIR}/artifacts/${SRC_PKG_NAME}-*/debian)
48- # Get the debian dir from the source package (involving the whole
49- # source tree can fail with 'No space left on device' on the phone).
50- TARGET_BASE=work
51- rm -fr ${TARGET_BASE}
52- mkdir -p ${TARGET_BASE}
53- cp -rd ${SOURCE_DIR} ${TARGET_BASE}
54- # Inject the boot DEP8 test into the debian dir from the package
55- # source tree
56- FROM=${TESTS}/boottest/debian/tests
57- TARGET="${TARGET_BASE}/debian/tests"
58- mkdir -p ${TARGET} # For packages that don't define DEP8 tests
59+ FROM=${TESTS}/boottest/debian/tests/control.template
60+ TARGET=${TESTS}/boottest/debian/tests/control
61+
62 # Inject the binary packages built previously
63 BIN_PACKAGES=$(tr '\n' ',' < ${PKG_SRC_DIR}/artifacts/needs_install.packages | sed -e s/,$//)
64- sed -e "s/{{ bin_packages }}/${BIN_PACKAGES}/" \
65- ${FROM}/control.template > ${TARGET}/control
66- cp ${FROM}/boottest ${TARGET}
67+ sed -e "s/{{ bin_packages }}/${BIN_PACKAGES}/" ${FROM} > ${TARGET}
68
69- # Now execute the boot test from inside the (reduced) pkg source tree
70+ # Now execute the boot test
71 set +e
72- ${ADT_CMD} --unbuilt-tree ${TARGET_BASE} -o results ${ADT_OPTS}
73+ ${ADT_CMD} --unbuilt-tree ${TESTS}/boottest -o results ${ADT_OPTS}
74 RET=$?
75 set -e
76 fi
77
78=== renamed directory 'tests/getpkgsrc' => 'tests/getinstalledpkgs'
79=== modified file 'tests/getinstalledpkgs/debian/changelog'
80--- tests/getpkgsrc/debian/changelog 2015-01-24 21:31:54 +0000
81+++ tests/getinstalledpkgs/debian/changelog 2015-06-02 16:14:14 +0000
82@@ -1,4 +1,4 @@
83-getpkgsrc (0.1) vivid; urgency=medium
84+getinstalledpkgs (0.1) vivid; urgency=medium
85
86 * Initial release.
87
88
89=== modified file 'tests/getinstalledpkgs/debian/control'
90--- tests/getpkgsrc/debian/control 2015-01-24 21:31:54 +0000
91+++ tests/getinstalledpkgs/debian/control 2015-06-02 16:14:14 +0000
92@@ -1,9 +1,9 @@
93-Source: getpkgsrc
94+Source: getinstalledpkgs
95 Section: misc
96 Maintainer: Canonical CI Engineering <canonical-ci-engineering@lists.launchpad.net>
97 Build-Depends: debhelper (>= 9),
98 Standards-Version: 3.9.4
99
100-Package: getpkgsrc
101+Package: getinstalledpkgs
102 Architecture: all
103 Description: A dep8 test providing a package source from inside the testbed.
104
105=== modified file 'tests/getinstalledpkgs/debian/tests/control'
106--- tests/getpkgsrc/debian/tests/control 2015-01-24 21:31:54 +0000
107+++ tests/getinstalledpkgs/debian/tests/control 2015-06-02 16:14:14 +0000
108@@ -1,2 +1,2 @@
109-Tests: getpkgsrc
110-Depends: dpkg-dev
111+Tests: getinstalledpkgs
112+Depends: dpkg-dev, dctrl-tools
113
114=== renamed file 'tests/getpkgsrc/debian/tests/getpkgsrc.template' => 'tests/getinstalledpkgs/debian/tests/getinstalledpkgs.template'
115--- tests/getpkgsrc/debian/tests/getpkgsrc.template 2015-01-28 14:30:28 +0000
116+++ tests/getinstalledpkgs/debian/tests/getinstalledpkgs.template 2015-06-02 16:14:14 +0000
117@@ -4,10 +4,7 @@
118 cd ${ADT_ARTIFACTS}
119 # List installed packages, removing the ':{arch}' suffix
120 dpkg-query -f '${binary:Package}\n' -W | sed -e 's/:.*$//' > installed.packages
121-# adt-run complains and fails the test if sdterr is not empty
122-# redirect stderr to preserve the output for reference
123-apt-get source {{ source_package }} 2> apt-get-source.stderr
124 # Get the defined binary packages
125-grep ^Package: {{ source_package }}*/debian/control | sed -e 's/^Package: //' | sort > binary.packages
126+grep-aptavail -X -n -S -sPackage {{ source_package }} | sort | uniq > binary.packages
127 # Get the binary packages already installed
128 comm -1 -2 binary.packages installed.packages > needs_install.packages

Subscribers

People subscribed via source and target branches