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

[Francis Ginther]

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/

[Evan (ev)]

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

review: Needs Information

[Evan (ev)]

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.

[Iain Lane (laney)]

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> ]

[Iain Lane (laney)]

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> ]

[Francis Ginther (fginther)]

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.

[Iain Lane (laney)]

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> ]

[Francis Ginther (fginther)]

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.

[Francis Ginther (fginther)]

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.

[Paul Larson (pwlars)]

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

=== modified file 'scripts/boottest.sh'
--- scripts/boottest.sh	2015-03-16 16:20:32 +0000
+++ scripts/boottest.sh	2015-06-02 16:14:14 +0000
@@ -58,7 +58,7 @@
     fi
 
     # Ensure we leave a usable phone
-    [ -z ${NODE_NAME} ] || test-runner/scripts/recover.py ${NODE_NAME}
+    [ -z ${NODE_NAME} ] || ${BASEDIR}/scripts/recover.py ${NODE_NAME}
 
     # Leave a parting message
     # (disable command tracing as it confuses the output)
@@ -146,16 +146,16 @@
     --- ${ADT_VIRT}"
 
 
-# Inject the package name into getpkgsrc DEP8 test
-FROM=${TESTS}/getpkgsrc/debian/tests/getpkgsrc.template
-TARGET=${TESTS}/getpkgsrc/debian/tests/getpkgsrc
+# Inject the package name into getinstalledpkgs DEP8 test
+FROM=${TESTS}/getinstalledpkgs/debian/tests/getinstalledpkgs.template
+TARGET=${TESTS}/getinstalledpkgs/debian/tests/getinstalledpkgs
 sed -e "s/{{ source_package }}/${SRC_PKG_NAME}/" ${FROM} > ${TARGET}
 
 # Execute a first test to get the package source tree from the testbed.
 PKG_SRC_DIR=pkgsrc
 rm -fr ${PKG_SRC_DIR} || true
 set +e
-${ADT_CMD} --unbuilt-tree ${TESTS}/getpkgsrc -o ${PKG_SRC_DIR} ${ADT_OPTS}
+${ADT_CMD} --unbuilt-tree ${TESTS}/getinstalledpkgs -o ${PKG_SRC_DIR} ${ADT_OPTS}
 RET=$?
 set -e
 
@@ -167,7 +167,7 @@
     echo "$RELEASE $ARCH $SRC_PKG_NAME" > $errfile
     [ -f "$errfile" ] && ${RSYNC} -a $errfile $RSYNC_DEST/${RELEASE}/tmp/ || true
     # Ensure we leave a usable phone
-    [ -z ${NODE_NAME} ] || test-runner/scripts/recover.py ${NODE_NAME}
+    [ -z ${NODE_NAME} ] || ${BASEDIR}/scripts/recover.py ${NODE_NAME}
 
     exit $RET
 fi
@@ -179,27 +179,16 @@
 	RET=$?
 	set -e
 else
-	SOURCE_DIR=$(ls -d ${PKG_SRC_DIR}/artifacts/${SRC_PKG_NAME}-*/debian)
-        # Get the debian dir from the source package (involving the whole
-        # source tree can fail with 'No space left on device' on the phone).
-        TARGET_BASE=work
-        rm -fr ${TARGET_BASE}
-        mkdir -p ${TARGET_BASE}
-        cp -rd ${SOURCE_DIR} ${TARGET_BASE}
-	# Inject the boot DEP8 test into the debian dir from the package
-	# source tree
-	FROM=${TESTS}/boottest/debian/tests
-	TARGET="${TARGET_BASE}/debian/tests"
-	mkdir -p ${TARGET} # For packages that don't define DEP8 tests
+	FROM=${TESTS}/boottest/debian/tests/control.template
+	TARGET=${TESTS}/boottest/debian/tests/control
+
         # Inject the binary packages built previously
         BIN_PACKAGES=$(tr '\n' ',' < ${PKG_SRC_DIR}/artifacts/needs_install.packages | sed -e s/,$//)
-        sed -e "s/{{ bin_packages }}/${BIN_PACKAGES}/" \
-            ${FROM}/control.template > ${TARGET}/control
-	cp ${FROM}/boottest ${TARGET}
+        sed -e "s/{{ bin_packages }}/${BIN_PACKAGES}/"  ${FROM} > ${TARGET}
 
-	# Now execute the boot test from inside the (reduced) pkg source tree
+	# Now execute the boot test
 	set +e
-	${ADT_CMD} --unbuilt-tree ${TARGET_BASE} -o results ${ADT_OPTS}
+	${ADT_CMD} --unbuilt-tree ${TESTS}/boottest -o results ${ADT_OPTS}
 	RET=$?
 	set -e
 fi
 
=== renamed directory 'tests/getpkgsrc' => 'tests/getinstalledpkgs'
=== modified file 'tests/getinstalledpkgs/debian/changelog'
--- tests/getpkgsrc/debian/changelog	2015-01-24 21:31:54 +0000
+++ tests/getinstalledpkgs/debian/changelog	2015-06-02 16:14:14 +0000
@@ -1,4 +1,4 @@
-getpkgsrc (0.1) vivid; urgency=medium
+getinstalledpkgs (0.1) vivid; urgency=medium
 
   * Initial release.
 
 
=== modified file 'tests/getinstalledpkgs/debian/control'
--- tests/getpkgsrc/debian/control	2015-01-24 21:31:54 +0000
+++ tests/getinstalledpkgs/debian/control	2015-06-02 16:14:14 +0000
@@ -1,9 +1,9 @@
-Source: getpkgsrc
+Source: getinstalledpkgs
 Section: misc
 Maintainer: Canonical CI Engineering <[e-mail]>
 Build-Depends: debhelper (>= 9),
 Standards-Version: 3.9.4
 
-Package: getpkgsrc
+Package: getinstalledpkgs
 Architecture: all
 Description: A dep8 test providing a package source from inside the testbed.
 
=== modified file 'tests/getinstalledpkgs/debian/tests/control'
--- tests/getpkgsrc/debian/tests/control	2015-01-24 21:31:54 +0000
+++ tests/getinstalledpkgs/debian/tests/control	2015-06-02 16:14:14 +0000
@@ -1,2 +1,2 @@
-Tests: getpkgsrc
-Depends: dpkg-dev
+Tests: getinstalledpkgs
+Depends: dpkg-dev, dctrl-tools
 
=== renamed file 'tests/getpkgsrc/debian/tests/getpkgsrc.template' => 'tests/getinstalledpkgs/debian/tests/getinstalledpkgs.template'
--- tests/getpkgsrc/debian/tests/getpkgsrc.template	2015-01-28 14:30:28 +0000
+++ tests/getinstalledpkgs/debian/tests/getinstalledpkgs.template	2015-06-02 16:14:14 +0000
@@ -4,10 +4,7 @@
 cd ${ADT_ARTIFACTS}
 # List installed packages, removing the ':{arch}' suffix
 dpkg-query -f '${binary:Package}\n' -W | sed -e 's/:.*$//' > installed.packages
-# adt-run complains and fails the test if sdterr is not empty
-# redirect stderr to preserve the output for reference
-apt-get source {{ source_package }} 2> apt-get-source.stderr
 # Get the defined binary packages
-grep ^Package: {{ source_package }}*/debian/control | sed -e 's/^Package: //' | sort > binary.packages
+grep-aptavail -X -n -S -sPackage {{ source_package }} | sort | uniq > binary.packages
 # Get the binary packages already installed
 comm  -1 -2 binary.packages installed.packages > needs_install.packages