Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
jhbuild plugin: new plugin #1298
Conversation
codecov-io
commented
May 4, 2017
•
Edited 1 time
-
codecov-io
May 4, 2017
-
May 4, 2017
Codecov Report
@@ Coverage Diff @@
## master #1298 +/- ##
=========================================
- Coverage 95.22% 94.7% -0.52%
=========================================
Files 221 223 +2
Lines 20447 20749 +302
Branches 1629 1687 +58
=========================================
+ Hits 19471 19651 +180
- Misses 688 800 +112
- Partials 288 298 +10
Continue to review full report at Codecov.
|
|
Nice work, I am not fluent in dhbuild but will find someone who is to do a first pass on this and see if it is all good. Thanks! |
jhenstridge
suggested changes
May 5, 2017
I've left some inline comments. I think this needs a bit more work, but looks promising. The main points are:
-
do you really want to deploy jhbuild within snaps, or just use it as a build tool?
-
do we need all the dependency magic (plus the probably incomplete list of exceptions) in the plugin?
-
can this work with the existing snapcraft-desktop-helpers cloud parts for the runtime side of things?
| - ] | ||
| - } | ||
| - ] | ||
| -} |
jhenstridge
May 5, 2017
Was there any particular reason to delete this file? Presumably it is used by people running Visual Studio Code, and doesn't harm anyone else.
diddledan
Jun 14, 2017
Contributor
I removed that because I figured I had accidentally created it and thus it only had stuff relevant for me. I'll restore it.
| + ], | ||
| + 'snap': [ | ||
| + ], | ||
| + } |
jhenstridge
May 5, 2017
This seems unnecessarily verbose. I think it would be more readable with each os.path.join invocation on a single line (or wrapped if it they are too long, as recommended in PEP 8). I'd also recommend sorting the paths in the list.
It can be useful to spread lists over multiple lines to reduce the chance of merge conflicts, but it seems likely the only changes we'll see here are adding or removing files: not adding or removing path components.
| + | ||
| + for path in files[stage]: | ||
| + self.assertIs(True, os.path.exists(path), | ||
| + "path '%s' does not exist" % path) |
jhenstridge
May 5, 2017
You can just use assertTrue() here. Good job including the custom message on the assertion, but could you also include the value of the stage variable?
| + snap | ||
| + for snap in os.listdir(os.getcwd()) | ||
| + if snap.startswith('test-jhbuild_') and snap.endswith('.snap') | ||
| + ]) |
jhenstridge
May 5, 2017
This might be more concise using the glob module. Something like:
self.assertNotEqual([], glob.glob('test-jhbuild_*.snap'))
| + main: | ||
| + command: jhbuild run cat $SNAP/share/doc/main-module/README | ||
| + dep: | ||
| + command: jhbuild run cat $SNAP/share/doc/dep-module/README |
jhenstridge
May 5, 2017
Looking at these commands, this seems to imply that jhbuild ends up inside the resulting snap. Is that really what you want? It means pulling in the Python runtime for packages that may not actually need it.
All it is doing in this case is setting a few environment variables, which can probably be done as easily via the snapcraft-desktop-helpers cloud parts. Wouldn't it make more sense to just use jhbuild to build the application, and leave it at that?
| + - main-module | ||
| + module-set: simple-jhbuild | ||
| + module-set-dir: $SNAPCRAFT_STAGE/.. | ||
| + jhbuild-archive: $SNAPCRAFT_STAGE/.. |
jhenstridge
May 5, 2017
Wouldn't it make more sense to search for the module-set file within whatever has been defined as the source for the part?
I assume $SNAPCRAFT_STAGE/.. is an attempt to get back to the top level of the project, which raises a bit of a red flag. Usually a part would use source: . if its source is co-located within the project.
| + should be skipped should be prefixed with '-', e.g. '-WebKit' | ||
| + - module-set: (default: gnome-world) the module set JHBuild should use | ||
| + - module-set-dir: the directory where the module set is located | ||
| + - snap-revision: the revision of the snap this part is included in |
jhenstridge
May 5, 2017
How would a developer know what to set this to? The usual process for creating a snap package is to build the .snap file with Snapcraft, then upload it to the store which then assigns a revision. How would a developer know this while they're writing their snapcraft.yaml file?
attente
May 5, 2017
•
Edited 1 time
-
attente
May 5, 2017
-
May 5, 2017
You mean the store can potentially overwrite the revision of the uploaded snap to something other than what the developer specified in the snapcraft.yaml? That's really a big problem for this commit then because it's making a pretty big assumption that the location of /snap/<snap-name>/<snap-revision> is fixed.
jhenstridge
May 6, 2017
The currently active revision of the snap will be available at /snap/$snap_name/current if you need a fixed path. Although using the $SNAP environment variable at runtime is better if possible.
| + | ||
| + - To add a shell for debugging your snap, add an extra app whose command is: | ||
| + jhbuild -f $SNAP/etc/jhbuildrc shell. You might also want to add gdb, | ||
| + strace, etc. to your list of stage-packages for debugging purposes. |
jhenstridge
May 5, 2017
For what it is worth, developers can inspect the runtime environment for their snap with snap run --shell snap_name.app_name.
| + logging.getLogger().setLevel(logging.INFO) | ||
| + logging_handler = logging.StreamHandler() | ||
| + logging_handler.setFormatter(LoggingFormatter()) | ||
| +""" |
jhenstridge
May 5, 2017
I would strongly recommend against including a patch like this here (especially as a string within the source).
I suggest trying to get jhbuild patched upstream to disable this check if some environment variable is set.
| + module[1:] | ||
| + for module in self.options.modules | ||
| + if module[0] == '-' | ||
| + ] |
jhenstridge
May 5, 2017
As before, you've got a lot of extra white space here that doesn't really help with readability. Try to follow the style of the surrounding code (i.e. PEP 8).
| + if input: | ||
| + kwargs['stdin'], stdout = os.pipe() | ||
| + os.write(stdout, input.encode()) | ||
| + os.close(stdout) |
jhenstridge
May 5, 2017
Presumably you can remove this if you get rid of the need to patch jhbuild? It looks like this only works because you only use it with inputs smaller than the pipe buffer: if you exceeded that, the write would block.
| + '%s=%s' % (quote(var), quote(str(env[var]))) | ||
| + for var in env | ||
| + if env[var] is not None | ||
| + ] + args, **kwargs) |
jhenstridge
May 5, 2017
The parent run() method takes all the arguments of subprocess.Popen, which includes an env keyword argument. This takes a dictionary representing the new environment.
So if you copy os.environ and make your changes, you can just pass that straight in. That avoids the need to try to quote things (which is almost certainly wrong, since the command is not being interpreted by sh).
| + :return: the packages that provide the file | ||
| + :rtype: set | ||
| + """ | ||
| + if not getattr(self, 'apt_file_updated', False): |
jhenstridge
May 5, 2017
You set this member in the constructor: why do you need to use getattr() with a default to read it?
| + :param str build_package: the package whose stage packages to find | ||
| + :return: the packages that should be staged | ||
| + :rtype: set | ||
| + """ |
jhenstridge
May 5, 2017
A lot of the magic here doesn't seem particularly unique to jhbuild, so does it really belong here?
Also, are there many cases where this is actually needed? By default, Snapcraft will pull in libraries from the host system to satisfy dependencies of the executables/libraries that have been primed. That won't handle non-library dependencies, but perhaps it is okay to let users manually stage them as they would with other part types?
| + | ||
| + :param list modules: the modules to find dependencies for | ||
| + """ | ||
| + os.makedirs(os.path.join(self.sourcedir, '.cache'), exist_ok=True) |
jhenstridge
May 5, 2017
If the files you are stashing in this directory are build artefacts, maybe place them under self.builddir?
| + self.run([ | ||
| + 'git', | ||
| + 'pull', | ||
| + ], cwd=repository) |
jhenstridge
May 5, 2017
If the user specifies an alternative jhbuild git repository, might it be simpler to just clone it to the same location you'd clone the master repo?
| + ], cwd=os.path.join( | ||
| + self.sourcedir, | ||
| + 'jhbuild', | ||
| + ), input=ROOT_PATCH) |
jhenstridge
May 5, 2017
I think it probably makes more sense to clone jhbuild into self.builddir rather than self.sourcedir, if we consider sourcedir to be "where the module set comes from".
| + jhbuildrc_file.write( | ||
| + 'module_autogenargs[\'gdk-pixbuf\'] = ' | ||
| + '\'--disable-gio-sniffing\'\n' | ||
| + ) |
jhenstridge
May 5, 2017
This might be a bit easier if you specify the whole configuration in a single go, and use the str.format() method to substitute in named parameters. Something like:
config = '''
moduleset = {module_set!r}
...
'''
jhbuildrc_file.write(config.format(module_set=self.options.module_set, ...))
attente
commented
May 5, 2017
|
The dependency magic and exception list could probably be removed in favour of what's already in JHBuild, but that would mean staging -dev packages, and at the time, I wanted to minimize what went into the final snap. But yeah, really the added complexity probably wasn't worth the effort. |
jhenstridge
commented
May 6, 2017
|
With how Snapcraft works right now, you can add For many library dependencies, this is sufficient: it's often only necessary to manually stage a library package if the library depends on additional data files found in that package. That's why I was asking how much of the dependency resolution magic in your plugin was necessary. |
attente
commented
May 6, 2017
|
I'm not sure if we're on the same page here, or if I'm fundamentally misunderstanding some magic snapcraft might be doing. What I meant was that JHBuild would end up putting |
jhenstridge
commented
May 8, 2017
|
What I'm saying is that in many cases it isn't necessary to stage either ... then the shared library from |
attente
commented
May 8, 2017
|
That kind of magic worries me though. Copying a shared library into the snap doesn't seem as complete as fulfilling a runtime dependency. What happens if |
|
I've got an updated version of this plugin which addresses some of the raised points. I'll be uploading soon (hopefully) to get further feedback |
added a commit
to diddledan/snapcraft
that referenced
this pull request
May 26, 2017
added a commit
to diddledan/snapcraft
that referenced
this pull request
May 29, 2017
added a commit
to diddledan/snapcraft
that referenced
this pull request
May 29, 2017
added a commit
to diddledan/snapcraft
that referenced
this pull request
May 29, 2017
|
Thanks for the update @diddledan. @attente, @jhenstridge: could you please review it again? |
elopio
requested changes
Jun 14, 2017
I left some style comments, and some questions.
This looks great, thanks for your contribution!
The code is not simple, which is ok because jhbuild doesn't look simple at all. But then, this needs to have unit tests for all the possible code paths.
| @@ -0,0 +1,97 @@ | ||
| +# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*- | ||
| +# | ||
| +# Copyright (C) 2016 Canonical Ltd |
| +import integration_tests | ||
| + | ||
| + | ||
| +class StoppableHTTPRequestHandler(http.server.SimpleHTTPRequestHandler): |
elopio
Jun 14, 2017
Member
Take a look at how we deal with fake servers in fixtures:
https://github.com/snapcore/snapcraft/blob/master/snapcraft/tests/fixture_setup.py#L253
You can reuse some things there, instead of implementing the quit in tear down yourself.
| + def setUpClass(cls): | ||
| + cls.port = 8000 | ||
| + handler = StoppableHTTPRequestHandler | ||
| + httpd = StoppableHTTPServer(("127.0.0.1", cls.port), handler) |
elopio
Jun 14, 2017
Member
another nit, please use single quotes for strings, for consistency with the rest of the code.
| + | ||
| + @classmethod | ||
| + def tearDownClass(cls): | ||
| + conn = http.client.HTTPConnection("127.0.0.1:%d" % cls.port) |
| + @classmethod | ||
| + def tearDownClass(cls): | ||
| + conn = http.client.HTTPConnection("127.0.0.1:%d" % cls.port) | ||
| + conn.request("QUIT", "/") |
| + cls.server.join() | ||
| + | ||
| + def test_snap(self): | ||
| + |
| + | ||
| + for path in files[stage]: | ||
| + self.assertTrue(os.path.exists(path), | ||
| + "path '%s' does not exist" % path) |
elopio
Jun 14, 2017
Member
In here it's ok to use double quotes, because you have a single quote inside. But please use .format.
Also, you can use the testtools assertion: http://testtools.readthedocs.io/en/latest/for-test-authors.html?highlight=fileexists#fileexists
I usually don't like fors in tests, but I like this one! :D
| + </systemdependencies> | ||
| + </systemmodule> | ||
| + <autotools id="dep-module" autogen-sh="autoreconf"> | ||
| + <branch version="1.0.0" module="dep-module.tar.bz2" checkoutdir="dep-module"/> |
elopio
Jun 14, 2017
Member
It would be nice to explain what are these binary files and how you generated them.
| +grade: stable | ||
| +confinement: strict | ||
| +summary: Simple JHBuild integration test | ||
| +description: > |
| @@ -0,0 +1,383 @@ | ||
| +# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*- | ||
| +# | ||
| +# Copyright (C) 2016 Canonical Ltd |
| + - module-set: (default: gnome-world) the module set JHBuild should use | ||
| + - module-set-dir: the directory where the module set is located | ||
| + Either leave unset to use the default moduleset definitions, or | ||
| + set to `.` to reference your project folder. |
| + - jhbuild-archive: the source tarball directory on the build host | ||
| + - jhbuild-mirror: the DVCS repository directory on the build host | ||
| + | ||
| +Advice: |
elopio
Jun 14, 2017
Member
This is a cool thing to add, I think!
What do you think @kyrofa @kalikiana ?
| +import snapcraft | ||
| +from snapcraft.internal import common | ||
| + | ||
| +HOME = os.path.expanduser('~') if os.geteuid() else os.path.join('/', 'root') |
elopio
Jun 14, 2017
Member
I'm not sure about why this is needed. Wouldn't os.environ.get('HOME') work for both root and non-root?
| +} | ||
| + | ||
| + | ||
| +LOG = logging.getLogger(__name__) |
| +LOG = logging.getLogger(__name__) | ||
| + | ||
| + | ||
| +def _get_jhbuild_user(): |
elopio
Jun 14, 2017
Member
a docstring in this function would be useful, because it's not clear for me why it's needed.
| +def _get_jhbuild_user(): | ||
| + myuid = os.getuid() | ||
| + if myuid != 0: | ||
| + return myuid |
elopio
Jun 14, 2017
Member
we don't prefix variables with my. uid seems like a good name for me here.
| + return uid | ||
| + | ||
| + | ||
| +def _get_jhbuild_group(): |
elopio
Jun 14, 2017
Member
as on the other, a docstring would be appreciated. and gid instead of mygid.
| +class JHBuildPlugin(snapcraft.BasePlugin): | ||
| + """ | ||
| + JHBuildPlugin provides jhbuild functionality to snapcraft | ||
| + """ |
elopio
Jun 14, 2017
Member
Another nit, we use a single line when the docstrings are a single line. No need for the """ in their own lines.
| + 'type': 'string', | ||
| + 'default': '', | ||
| + }, | ||
| + 'cflags': { |
| + self.partdir, 'jhbuild', 'usr', 'bin', 'jhbuild') | ||
| + self.jhbuildrc_path = os.path.join(self.partdir, 'jhbuildrc') | ||
| + | ||
| + def enable_cross_compilation(self): |
elopio
Jun 14, 2017
Member
This is the default from the base class, so no need to define it in this plugin.
| + 'The {!s} plugin does not support cross compilation'.format( | ||
| + self.name)) | ||
| + | ||
| + def run(self, cmd, cwd=None, **kwargs): |
elopio
Jun 14, 2017
Member
Why are you defining run and run_output only to call the definitions from the supper class? It would be the same to just remove them from this class.
| + """ | ||
| + return super().run_output(cmd, cwd=cwd, **kwargs) | ||
| + | ||
| + def jhbuild(self, args, output=True, **kwargs): |
| + return super().run_output(cmd, cwd=cwd, **kwargs) | ||
| + | ||
| + def jhbuild(self, args, output=True, **kwargs): | ||
| + """Run JHBuild in the build stage. |
| + :return: the output of the command if captured | ||
| + :rtype: str | ||
| + """ | ||
| + |
| + if self.options.jhbuild_mirror: | ||
| + repository = self.options.jhbuild_mirror | ||
| + | ||
| + if os.path.isdir(self.jhbuild_src): |
elopio
Jun 14, 2017
Member
is the jhbuild_src dir always a git repository? And why do we need it in our own directory instead of calling it from the directory defined in the property?
| + | ||
| + def pull(self): | ||
| + self._pull_jhbuild() | ||
| + self._setup_jhbuild() |
elopio
Jun 14, 2017
Member
It's nicer when you are able to read the source code from top to bottom. For that, please move the definition of _pull_jhbuild, _setup_jhbuild and jhbuild after this pull method.
Then somebody new to the code will find first the pull method, and if he wants to dig further, he will also read the definitions of the other 3 methods. If he only wants a high level overview, he will just read pull. With the current implementation he will find first the definition of setup_jhbuild with no context to know if it's an implementation detail, or something critical to understand what the plugin does.
| + | ||
| + if 'gtk+' in modules or 'gtk+-3' in modules: | ||
| + self.modules.append('adwaita-icon-theme') | ||
| + self.stage_packages.append('ttf-ubuntu-font-family') |
| + cwd=self.builddir, output=False) | ||
| + | ||
| + LOG.info('Fixing symbolic links') | ||
| + self.run(['symlinks', '-c', '-d', '-r', '-s', '-v', self.installdir]) |
elopio
Jun 14, 2017
Member
could you please explain with a comment in the source code why this is needed?
kyrofa
Jun 14, 2017
Member
Whoa, cool tool, we have a lot of symlink logic elsewhere, just in Python-- I wonder if we should start using this instead. @elopio, thoughts?
diddledan
Jun 14, 2017
Contributor
I'll remove the usage of symlinks on the premise that snapcraft will "do the right thing" - that might mean including the symlinks command elsewhere in snapcraft.
kyrofa
Jun 14, 2017
Member
That's not what I meant by my comment-- you may very well need to fix these up. @elopio was just wanting a comment in the code as to why.
| @@ -0,0 +1,53 @@ | ||
| +# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*- | ||
| +# | ||
| +# Copyright (C) 2016 Canonical Ltd |
Indeed, this is why Snapcraft 3.x will stop doing that: it's not clear at what point the magic stops! Currently snapcraft literally runs |
| - ] | ||
| - } | ||
| - ] | ||
| -} |
jhenstridge
May 5, 2017
Was there any particular reason to delete this file? Presumably it is used by people running Visual Studio Code, and doesn't harm anyone else.
diddledan
Jun 14, 2017
Contributor
I removed that because I figured I had accidentally created it and thus it only had stuff relevant for me. I'll restore it.
| + - jhbuild-archive: the source tarball directory on the build host | ||
| + - jhbuild-mirror: the DVCS repository directory on the build host | ||
| + | ||
| +Advice: |
elopio
Jun 14, 2017
Member
This is a cool thing to add, I think!
What do you think @kyrofa @kalikiana ?
| + | ||
| + - The desktop-gtk3 helper is incompatible with GTK built by jhbuild. A custom | ||
| + helper is currently required. | ||
| + See github.com/diddledan/corebird for example. |
kyrofa
Jun 14, 2017
Member
Linking to a personal project in snapcraft docs seems like a recipe for dead links
diddledan
Jun 14, 2017
Contributor
The problem is that the desktop-launch script which gets built by desktop-gtk3 requires multiple changes to work around the different installation locations of ubuntu's packages vs jhbuild-built packages.
| +import snapcraft | ||
| +from snapcraft.internal import common | ||
| + | ||
| +HOME = os.path.expanduser('~') if os.geteuid() else os.path.join('/', 'root') |
elopio
Jun 14, 2017
Member
I'm not sure about why this is needed. Wouldn't os.environ.get('HOME') work for both root and non-root?
| + }, | ||
| + 'module-set': { | ||
| + 'type': 'string', | ||
| + 'default': 'gnome-world', |
kyrofa
Jun 14, 2017
Member
Not a dhbuild guy here, but what happens if this is an empty string, or None? Even though it has a default, should it be required?
| + }, | ||
| + } | ||
| + | ||
| + schema['required'] = ['modules'] |
kyrofa
Jun 14, 2017
Member
The BasePlugin makes this a list already. Since plugins can build on each other, I suggest maintaining the practice of appending to this list as opposed to setting it again (see the other plugins for reference).
| + @classmethod | ||
| + def get_pull_properties(cls): | ||
| + # Inform Snapcraft of the properties associated with pulling. If these | ||
| + # change in the YAML Snapcraft will consider the build step dirty. |
| + module for module in self.options.modules if module[0] != '-'] | ||
| + | ||
| + self.skip_modules = [ | ||
| + module[1:] for module in self.options.modules if module[0] == '-'] |
kyrofa
Jun 14, 2017
Member
This doesn't account for spaces at all (e.g. "- WebKit"). Would it be a good idea to strip this, or is "-WebKit" the only format allowed?
| + return super().run_output(cmd, cwd=cwd, **kwargs) | ||
| + | ||
| + def jhbuild(self, args, output=True, **kwargs): | ||
| + """Run JHBuild in the build stage. |
| + return run(cmd + args, cwd=cwd, **kwargs) | ||
| + | ||
| + def _create_jhbuild_user(self): | ||
| + common.run(['adduser', '--shell', '/bin/bash', '--disabled-password', |
kyrofa
Jun 14, 2017
Member
Does this potentially require sudo, then? Might be worth mentioning in the docs at the top. In fact, it seems like a run may or may not fail depending on whether or not it was run with sudo and whether or not that user already exists. Should we instead use sudo, here, thus prompting for a password if necessary?
I don't like creating users under the covers like this, by the way, but I assume it's required for jhbuild.
diddledan
Jun 14, 2017
Contributor
jhbuild refuses to run under the root account, so we need to sudo to an unprivileged user when cleanbuilding. I think we can limit the sudo and adduser calls to only running when the current euid is 0.
| + def _pull_jhbuild(self): | ||
| + LOG.info('Pulling JHBuild') | ||
| + | ||
| + repository = 'https://git.gnome.org/browse/jhbuild' |
| + cwd=self.builddir, output=False) | ||
| + | ||
| + LOG.info('Fixing symbolic links') | ||
| + self.run(['symlinks', '-c', '-d', '-r', '-s', '-v', self.installdir]) |
elopio
Jun 14, 2017
Member
could you please explain with a comment in the source code why this is needed?
kyrofa
Jun 14, 2017
Member
Whoa, cool tool, we have a lot of symlink logic elsewhere, just in Python-- I wonder if we should start using this instead. @elopio, thoughts?
diddledan
Jun 14, 2017
Contributor
I'll remove the usage of symlinks on the premise that snapcraft will "do the right thing" - that might mean including the symlinks command elsewhere in snapcraft.
kyrofa
Jun 14, 2017
Member
That's not what I meant by my comment-- you may very well need to fix these up. @elopio was just wanting a comment in the code as to why.
added a commit
to diddledan/snapcraft
that referenced
this pull request
Jun 14, 2017
added a commit
to diddledan/snapcraft
that referenced
this pull request
Jun 15, 2017
added a commit
to diddledan/snapcraft
that referenced
this pull request
Jul 7, 2017
diddledan
added some commits
May 3, 2017
added a commit
to diddledan/corebird-snap
that referenced
this pull request
Aug 4, 2017
|
Thanks for all the changes @diddledan. I changed my review to +1, and now we only need to wait for @jhenstridge to approve this. |
sergiusens
changed the title from
Reroll of proposed JHBuild plugin
to
jhbuild plugin: new plugin
Aug 10, 2017
added a commit
to diddledan/corebird-snap
that referenced
this pull request
Aug 25, 2017
jhenstridge
commented
Aug 29, 2017
|
Sorry for the delays. This looks like it corrects the main problems I had with the initial versions of the plugin (shipping a diff against jhbuild itself, and creating a new system for discovering dependencies specific to the plugin). So I've got no objections now. |
sergiusens
and others
added some commits
Sep 4, 2017
sergiusens
merged commit f5a51ee
into
snapcore:master
Sep 5, 2017
1 check passed
sergiusens
added this to the 2.34 milestone
Sep 5, 2017
added a commit
to kalikiana/snapcraft
that referenced
this pull request
Sep 21, 2017
added a commit
to diddledan/corebird-snap
that referenced
this pull request
Nov 19, 2017
diddledan commentedMay 4, 2017
LP: #1643924
Previous PR: #924
The previous PR was closed due to the attempt relying on a package which isn't available in Xenial. I have taken that PR (props to @attente for the original work, which I have based this PR on) and removed the dependency on Bubblewrap. I then massaged the code to simplify and unify duplicated code. The tests pass on my Xenial LXC container and also pass Travis-CI's suite.