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

powerdns-admin: fix build #161896

Closed
wants to merge 2 commits into from
Closed

powerdns-admin: fix build #161896

wants to merge 2 commits into from

Conversation

Flakebi
Copy link
Member

@Flakebi Flakebi commented Feb 25, 2022

Motivation for this change

Since the last python updates, powerdns-admin failed to build.

Fix the dnspython pin (it was updated) and pin jsonschema in bravado-core to fix the build.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Copy link
Member

@zhaofengli zhaofengli left a comment

Choose a reason for hiding this comment

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

Tested to work 🚀 I haven't got a chance to look at the breakage since the last update, thank you for getting that sorted out!

Comment on lines +6 to +11
let
jsonschema320 = jsonschema.overridePythonAttrs (oldAttrs: rec {
version = "3.2.0";

src = fetchPypi {
inherit (oldAttrs) pname;
Copy link
Contributor

Choose a reason for hiding this comment

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

Pinning within python-modules is highly discouraged. This will potentially introduce incompatible versions in a user's environment as either the new pinned version or the original version will be imported at runtime, breaking one of the packages.

The preference to handling this is to relax version bounds in the "install_requires" field. (could be in setup.py, pyproject.toml, requirements or others). In most cases, packages are still compatible with small API changes which may warrant a major version bump. We use test suites to verify that the package still works correctly.

If the package is still incompatible with the latest major version, then the most proper way to handle this is make an issue with the upstream package to adopt the latest major version. Or if upstream is not very responsive, you are free patch the source to make it compatible.

In very few circumstances, two versions of the same package are allowed to exist if the packages are extremely difficult to package. Some examples of this are tensorflow, which has huge ecosystems built around it and is hard to package. Another is django, which has 2 actively developed versions, and large ecosystems built around each.

One exception to this is applications, due to the way buildPythonApplication and toPythonApplication functions work, the related derivations will not bleed dependencies between packages. If the package doesn't need to be imported by other python modules, then this package would be a good candidate to convert into application. You can look at https://github.com/NixOS/nixpkgs/blob/master/pkgs/tools/admin/awscli/default.nix as an example of using an overlay within a python application.

Another valid exception is pinning within the same interpreter version. If a package no longer supports older-but-still-relevant interpreter versions, then you're free to pin a package for an interpreter. This was very common with python2, but less common amongst python3 interpreters. (e.g. if (pythonOlder "3.8" then <older pkg> else <latest package> ;)

Info on buildPythonApplication can be found here.
Info on toPythonApplication can be found here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like bravado-core does not work with jsonschema 4, there’s an upstream PR to pin it to <4 because of bugs: Yelp/bravado-core#385
As bravado-core is a library, I also don’t know a way to override it for all dependencies once, therefore the override of swagger-spec-validator to not end up with two different versions of jsonschema (which does not compile).

@Artturin Artturin added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels May 11, 2022
@Flakebi Flakebi mentioned this pull request May 24, 2022
13 tasks
@Flakebi
Copy link
Member Author

Flakebi commented May 24, 2022

Powerdns-admin was fixed differently in #174329

@Flakebi Flakebi closed this May 24, 2022
@Flakebi Flakebi deleted the powerdns-admin branch May 24, 2022 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: python 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants