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

Jump to bottom

buildbot: 2.4.1 -> 2.5.0 #72059

Merged
merged 1 commit into from Nov 8, 2019
Merged

buildbot: 2.4.1 -> 2.5.0 #72059

merged 1 commit into from Nov 8, 2019

Conversation

lopsided98
Copy link
Contributor

Motivation for this change

Updates buildbot to the latest version.

Buildbot has acquired some new test dependencies, including a circular dependency on the buildbot_www plugin, which requires using a wheel (as was done in the past).

Things done
  • 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 nix-review --run "nix-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @nand0p @ryansydnor

@lopsided98

This comment has been minimized.

Sign in to view
@lopsided98
Copy link
Contributor Author

x86_64-linux build always fails on ofborg.

@veprbl
Copy link
Member

veprbl commented Oct 27, 2019

x86_64-linux build always fails on ofborg.

Usually only the test was failing. This time it's the build fails on the tests that somehow are sensitive to a warning:

builtins.DeprecationWarning: The usage of `cmp` is deprecated and will be removed on or after 2021-06-01.  Please use `eq` and `order` instead.

This is not an issue for aarch64 because there we don't run tests:

nixpkgs/pkgs/development/python-modules/buildbot/default.nix

Lines 81 to 82 in 7edc64b

# TimeoutErrors on slow machines -> aarch64
doCheck = !stdenv.isAarch64;

veprbl
veprbl reviewed Oct 27, 2019
View reviewed changes
pkgs/development/python-modules/buildbot/plugins.nix
};

buildInputs = [ buildbot buildbot-pkg mock ];
Copy link
Member

Choose a reason for hiding this comment

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

These should have been checkInputs? If yes, we could use buildbot.overrideAttrs { doCheck = false; checkInputs = []; }.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, if they are not available, the build fails with:
Please install buildbot, buildbot_pkg, and mock modules in order to install that package, or use the pre-build .whl modules available on pypi

See: https://github.com/buildbot/buildbot/blob/master/www/base/setup.py#L24

FRidh
FRidh reviewed Oct 27, 2019
View reviewed changes
pkgs/development/python-modules/buildbot/plugins.nix Outdated
};

buildInputs = [ buildbot buildbot-pkg mock ];
format = "wheel";
Copy link
Member

Choose a reason for hiding this comment

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

avoid wheels when possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only way I can see to avoid a wheel is to patch the package's setuptools script.

Copy link
Member

Choose a reason for hiding this comment

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

A comment explaining that should be added.
How about using pipInstallFlags = [ "--ignore-dependencies" ]; instead?

@lopsided98
Copy link
Contributor Author

The tests passed locally when I wrote this PR against unstable, but broke due to a dependency upgrade when rebased against master. The issue (glyph/automat#117) has not been fixed upstream, so I applied a patch used by Arch Linux to ignore the warning for now.

I was not able to find a way to stop buildbot-plugins.www from depending on buildbot other than patching setup.py with sed, so that is what I ended up doing. Using --ignore-dependencies (or other flags) had no effect, even after removing the custom import check.

None of the plugins actually have any Python tests (www has Javascript tests that we don't run), so I simply set doCheck = false for all of them, removing the circular dependency. I was asked to do this in the past, but at the time thought it wasn't a good idea. The only reason buildbot was needed as a checkInput is that the __init__.py for each plugin imports it, and this is run even if there are no tests.

@lopsided98

This comment has been minimized.

Sign in to view
@lopsided98
Copy link
Contributor Author

@GrahamcOfBorg test buildbot

@FRidh
Copy link
Member

FRidh commented Oct 28, 2019

Thanks for the explanation. Could you rebase your changes?

@jonringer
Copy link
Contributor

@GrahamcOfBorg build buildbot-full

jonringer
jonringer approved these changes Nov 2, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM
executables seem to work
not sure how to test plugins

[4 built, 2 copied (0.7 MiB), 0.2 MiB DL]
https://github.com/NixOS/nixpkgs/pull/72059
13 package were build:
buildbot buildbot-full buildbot-ui python37Packages.buildbot-plugins.console-view python37Packages.buildbot-plugins.grid-view python37Packages.buildbot-plugins.waterfall-view python37Packages.buildbot-plugins.wsgi-dashboards python37Packages.buildbot-plugins.www python38Packages.buildbot-plugins.console-view python38Packages.buildbot-plugins.grid-view python38Packages.buildbot-plugins.waterfall-view python38Packages.buildbot-plugins.wsgi-dashboards python38Packages.buildbot-plugins.www

@lopsided98
Copy link
Contributor Author

Can this be merged?

@jonringer
Copy link
Contributor

review still passes, merging :)

@jonringer jonringer merged commit a1c3a7f into NixOS:master Nov 8, 2019
@lopsided98 lopsided98 deleted the buildbot-update branch November 8, 2019 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veprbl veprbl veprbl left review comments

@FRidh FRidh FRidh left review comments

@jonringer jonringer jonringer approved these changes

Assignees
No one assigned
Labels
6.topic: python 8.has: clean-up 8.has: package (new) 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lopsided98 @veprbl @FRidh @jonringer