Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
tests: dependency packages installed during prepare-project #3483
Conversation
codecov-io
commented
Jun 15, 2017
•
Edited 1 time
-
codecov-io
Jun 19, 2017
-
Jun 19, 2017
Codecov Report
@@ Coverage Diff @@
## master #3483 +/- ##
==========================================
+ Coverage 74.9% 74.91% +<.01%
==========================================
Files 380 380
Lines 32951 32986 +35
==========================================
+ Hits 24682 24711 +29
- Misses 6476 6483 +7
+ Partials 1793 1792 -1
Continue to review full report at Codecov.
|
sergiocazzolato
changed the title from
tests: dependency packages installed during project prepare
to
tests: dependency packages installed during prepare-project
Jun 15, 2017
|
Thanks for doing this! It looks good, I wonder however if dependencies.sh should instead simply be part of pkgdb.sh, this way all pkg releated things can be easily found in the same place. |
niemeyer
requested changes
Jun 19, 2017
Thanks! Going in a good direction. Just a few trivial points and questions.
| @@ -0,0 +1,131 @@ | ||
| +#!/bin/bash |
niemeyer
Jun 19, 2017
Contributor
Agree with @mvo5 that pkgdb.sh seems like a reasonable place for this logic.
| +export DISTRO_BUILD_DEPS= | ||
| + | ||
| +add_pkgs(){ | ||
| + DEPENDENCY_PACKAGES="$DEPENDENCY_PACKAGES $@" |
niemeyer
Jun 19, 2017
Contributor
We're trying to standardize on four spaces per indent on shell and yaml code. I've seen some other files which are already all over the place, with mixed 2, 3 and 4 spaces on the same file. Let's please fix that as we drive by these files (no need to rush it), and also try not to introduce new inconsistencies meanwhile.
| +} | ||
| + | ||
| +get_apt_dependencies_generic(){ | ||
| + add_pkgs autoconf |
niemeyer
Jun 19, 2017
Contributor
Communicating across shell functions via sticky environment variables is pretty hard to follow. It's a backdoor without a clear flow between the involved parties. Instead, this can simply echo autoconf here, and the other side can use the standard output as usual for its needs.
| + esac | ||
| +} | ||
| + | ||
| +get_apt_dependencies_core(){ |
niemeyer
Jun 19, 2017
Contributor
I suggest s/get_/pkg_/ on all of these functions. This preserves them with the same length and adds a bit of context to it (e.g. "test dependencies" alone could mean many things).
| + echo "Opensuse dependencies not ready yet" | ||
| +} | ||
| + | ||
| +get_test_dependencies(){ |
niemeyer
Jun 19, 2017
Contributor
Is there an advantage in splitting down test and build? Won't we end up with both of these installed anyway?
|
Looks like the debian failures are related to your changes. |
|
@niemeyer when you have time, please take a look to this one, all the comments were addressed. |
| - *) | ||
| - ;; | ||
| -esac | ||
| +pkg_apt_dependencies_generic(){ |
fgimenez
Jul 17, 2017
•
Edited 1 time
-
fgimenez
Jul 17, 2017
Contributor
-
Jul 17, 2017
I find this naming a bit inconsistent with what we have for fedora and opensuse where there's no reference to dnf or zypper, maybe pkg_dependency_ubuntu_{generic,classic} would be better.
niemeyer
approved these changes
Jul 17, 2017
A few last comments, and LGTM with those addressed.
Thanks!
| -esac | ||
| + | ||
| +pkg_dependency_ubuntu_generic(){ | ||
| + echo autoconf |
| + echo rpm-build | ||
| +} | ||
| + | ||
| +pkg_dependency_opensuse(){ |
niemeyer
Jul 17, 2017
Contributor
s/dependency/dependencies/ on all cases above would read slightly better and also align with the cases below.
| + esac | ||
| +} | ||
| + | ||
| +install_dependencies(){ |
| +} | ||
| + | ||
| +install_dependencies(){ | ||
| + pkgs=$(pkg_dependencies | tr "\n" " ") |
| + pkgs=$(pkg_dependencies | tr "\n" " ") | ||
| + | ||
| + # ensure systemd is up-to-date, if there is a mismatch libudev-dev | ||
| + # will fail to install because the poor apt resolver does not get it |
niemeyer
Jul 17, 2017
Contributor
As such, this should be inside distro_install_package and should be conditional on libudev-dev being part of the requested set.
| + apt-get install -y --only-upgrade systemd | ||
| + esac | ||
| + | ||
| + echo "Installing the following packages: $pkgs" |
|
@niemeyer fixes requested already pushed |
sergiocazzolato
and others
added some commits
Jun 14, 2017
sergiocazzolato commentedJun 14, 2017
The objective of this change is to provide an environment for all the
tests as similar as possible, so all the dependencies are installed
during the project setup.
The packages are not removed at the end of the project, so it will help
in case the machine needs to be used to debug.
Openvswitch test is moved to manual due to the dependencies install is
stuck trying to install openvswitch-switch package.