Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buildbot: Python 3 support and other improvements #45890

Merged
merged 10 commits into from
Oct 12, 2018

Conversation

lopsided98
Copy link
Contributor

@lopsided98 lopsided98 commented Sep 1, 2018

Motivation for this change

The latest versions of pylint and astroid do not support Python 2, which breaks buildbot (#45782) because it only supports Python 2 in nixpkgs. Buildbot supports Python 3, but this is potentially a breaking change because buildbot uses Python in its configuration files. Therefore, this PR adds configurable support for Python 2 or 3, which Python 2 being the default. The 1.0 branches of pylint and astroid are still supported until Python 2's EOL, so they are used to build buildbot with Python 2.

This PR supersedes #45789.

Things done

First, I updated and added Python 3 support to sphinx-jinja. This mainly required fixing how the tests were run. This commit supersedes #45763.

I updated tempita to use a fork that adds Python 3 support. The upstream tempita package appears to be abandoned. This fork is used by the official Arch Linux package of tempita, but is somewhat undesirable because it is not an official release. This commit supersedes #39692.

I updated astroid and pylint to their latest versions, and added astroid1 and pylint1 packages which use the still maintained 1.0 branch of these packages. Because there are a number of different ways to implement packages with multiple versions in Nix, I'm not sure if I chose the best way.

While I was testing I ran into #40273, which is supposedly fixed but does not seem to be (see #40273 (comment)). To work around this I changed hyperlink to not use pytest to run its tests (the default checkPhase seems to work fine) and made some changes to autobahn (which do not fix the problem, but only one package needs to be fixed for the conflict to be resolved). These changes are not critical, because they should not be necessary once #40273 is truly fixed, but I included them anyway because I believe they result in cleaner derivations.

I then updated all the components of buildbot to 1.3.0. buildbot-worker had already been updated by an automated update, but the rest of the components had not. This has happened a number of times recently and sometimes causes problems, especially when buildbot-pkg is updated without updating the plugins.

The last commit adds Python 3 support as well as a number of other improvements that I have been running locally for about a year. There are no intentional breaking changes, but I have not tested every possible scenario.

All the buildbot packages are moved to pythonPackages. A new python attribute is added to each package's passthru, allowing the NixOS module to determine which version of Python to use. The tests are currently disabled for both Python 2 and 3, because a test fails due to what appears to be a bug in buildbot. I attempted to disable the failing test, but this simply causes the error to move to a different test and so on. The error message (see comment above doCheck = false) relates to a recently added buildbot feature, so this points to a problem with buildbot.

I changed how the plugin wrapper works, making the wrapped package into a Python module that can be added to a Python environment using python.withPackages.

In the buildbot-master NixOS module, I switched to directly invoking twistd (as was already done with buildbot-worker) in order to log to stdout and therefore the systemd journal, rather than a file. I also added a new pythonPackages option, which takes a function that is passed to python.withPackages and is used to set the service's PYTHONPATH. I also generate the twisted tac file within the module, rather than using create-master (create-master is still necessary to create the database). This gives more control over the initialization process (especially useful for buildbot-worker).

In the buildbot-worker NixOS module, I made similar improvements to the PYTHONPATH handling, but did not add a pythonPackages option because workers never run user Python code AFAIK. I also generate the tac file in the module, which makes it possible to implement the workerPassFile option (checks one of the boxes in #24288). I also added hostMessage and adminMessage options, which allow users to declaratively set those messages.

The buildbot NixOS test passes, including the commented out daemon test, which supposedly was previously broken. I configured the tests to run with both Python 2 and 3.

I would appreciate it if other buildbot users would test this PR. I have only run the tests and used it with my buildbot deployment, which has a number of customizations with might obscure bugs.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@lopsided98 lopsided98 mentioned this pull request Sep 1, 2018
9 tasks
@GrahamcOfBorg GrahamcOfBorg added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500 labels Sep 1, 2018
@xeji
Copy link
Contributor

xeji commented Sep 1, 2018

cc @dotlambda

nixos/tests/buildbot.nix Show resolved Hide resolved
pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
ln -sfv ${package}/lib $out/lib
'';
# Can't use runCommand because it does not support propagatedBuildInputs
withPlugins = plugins: toPythonModule (stdenv.mkDerivation {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you use buildPythonPackage?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can simply be python.withPackages.

pkgs/development/tools/build-managers/buildbot/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/build-managers/buildbot/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/sphinx-jinja/default.nix Outdated Show resolved Hide resolved
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether we really should move buildbot into python-packages, however, if we do this, then the expressions should live there as well (python-modules/), and more importantly, no expressions should use buildPythonApplication.

pkgs/development/python-modules/astroid/1.6.nix Outdated Show resolved Hide resolved
propagatedBuildInputs = [ six twisted zope_interface txaio ] ++
(stdenv.lib.optional isPy33 asyncio) ++
(stdenv.lib.optionals (!isPy3k) [ trollius futures ]);

# Upstream claim python2 support, but tests require pytest-asyncio which
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trollius 2 is needed for asyncio support

pkgs/development/python-modules/pylint/1.9.nix Outdated Show resolved Hide resolved
ln -sfv ${package}/lib $out/lib
'';
# Can't use runCommand because it does not support propagatedBuildInputs
withPlugins = plugins: toPythonModule (stdenv.mkDerivation {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can simply be python.withPackages.

@dotlambda
Copy link
Member

I'm not sure whether we really should move buildbot into python-packages

I still think that it is easier if we put it in pythonPackages because otherwise, multiple packages, e.g. buildbot, buildbot-worker, ... have to be overridden to use a different Python version.
However, I'm not sure whether we should put an alias in all-packages.nix.
Anyway, this needs a changelog entry.

@lopsided98 lopsided98 force-pushed the buildbot-python3 branch 2 times, most recently from 856b4b4 to 6586375 Compare September 3, 2018 04:13
@GrahamcOfBorg GrahamcOfBorg added 8.has: changelog 8.has: documentation 2.status: merge conflict This PR has merge conflicts with the target branch labels Sep 3, 2018
@lopsided98 lopsided98 force-pushed the buildbot-python3 branch 3 times, most recently from 73c3ff0 to 87a3737 Compare September 4, 2018 19:15
@GrahamcOfBorg GrahamcOfBorg removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 4, 2018
@lopsided98
Copy link
Contributor Author

I think I have addressed all the issues that were brought up. I had to add a few new commits to fix autobahn's tests with Python 2, and a commit to add a patch to sqlalchemy_migrate to make it compatible with Python 3.6 (I previously had this in an overlay and forgot about it).

@nand0p I kept you as the maintainer of the 1.x branches of astroid and pylint. Is this okay?

Buildbot just updated to 1.4.0, and its tests now pass for Python 2 and 3.

@lopsided98
Copy link
Contributor Author

I rebased against master and moved the changelog entry to the 19.03 changelog.

@lopsided98 lopsided98 force-pushed the buildbot-python3 branch 5 times, most recently from cf05986 to 86c10a7 Compare October 11, 2018 20:55
@infinisil
Copy link
Member

@GrahamcOfBorg test buildbot.python2 buildbot.python3

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: tests.buildbot.python2, tests.buildbot.python3

Partial log (click to expand)

Oct 11 22:13:51 bbworker mqa4mvwqafrn53kcwzvxysr4p5llcpmx-unit-script-buildbot-worker-pre-start[753]: mkdir: created directory '/home/bbworker/worker/info'
Oct 11 22:13:51 bbworker systemd[1]: Started Buildbot Worker..
total 12
drwxr-xr-x 3 bbworker bbworker 4096 Oct 11 22:13 .
drwx------ 3 bbworker bbworker 4096 Oct 11 22:13 ..
drwxr-xr-x 2 bbworker bbworker 4096 Oct 11 22:13 info
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/7mnda9bfn2czxnhw4p42hbw4gviwdhb8-vm-test-run-buildbot
/nix/store/qja43p34slaqchzr0pdr5jcd8s90szjn-vm-test-run-buildbot

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: tests.buildbot.python2, tests.buildbot.python3

Partial log (click to expand)

Oct 11 22:58:14 bbworker systemd[1]: Starting Buildbot Worker....
Oct 11 22:58:14 bbworker systemd[1]: Started Buildbot Worker..
total 12
drwxr-xr-x 3 bbworker bbworker 4096 Oct 11 22:58 .
drwx------ 3 bbworker bbworker 4096 Oct 11 22:58 ..
drwxr-xr-x 2 bbworker bbworker 4096 Oct 11 22:58 info
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/2p09g21258a4dahpyqr9a598gpaif8ij-vm-test-run-buildbot
/nix/store/skbj8hg7in1581fhzm0wll0ycc6w4dd1-vm-test-run-buildbot

@lopsided98
Copy link
Contributor Author

@infinisil I fixed the issues you brought up.

Should this go into staging?

Also, there may be some buildbot users on 18.03 who will find their configs broken when they upgrade to 18.09, so we may want to backport this eventually. I'm not sure if we would backport the whole PR, or just the minimum to fix the build.

@infinisil
Copy link
Member

I think master is alright. Feel free to open another PR with a backport if you feel it's necessary.

Thanks for the PR, the NixOS test is very nice too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 8.has: changelog 8.has: clean-up 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants