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

python3.pkgs.sphinxcontrib-openapi: unbreak, switch to cilium fork #188280

Merged
merged 3 commits into from
Aug 30, 2022

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Aug 25, 2022

Description of changes

This fork uses m2r instead of mdinclude.

m2r is unmaintained and incompatible with recent versions of mistune (and old ones are insecure).

See

for details.

This PR also packages picobox and sphinx_mdinclude, which are new dependencies of sphinxcontrib-openapi.

Due to picobox being incompatible with Python 3.10 currently, this only unbreaks python39.pkgs.sphinxcontrib-openapi, but it's better than not being able to render openapi stuff at all.

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.11 Release Notes (or backporting 22.05 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.

@flokli flokli requested a review from SuperSandro2000 August 25, 2022 11:06
@flokli flokli requested review from FRidh and jonringer as code owners August 25, 2022 11:06
@flokli flokli changed the title python.pkgs.sphinxcontrib-openapi: unbreak, switch to cilium fork python39.pkgs.sphinxcontrib-openapi: unbreak, switch to cilium fork Aug 25, 2022
@ofborg ofborg bot added 8.has: clean-up 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Aug 25, 2022
@flokli
Copy link
Contributor Author

flokli commented Aug 25, 2022

Hmmh, this still seems to fail to execute, showing a

AttributeError: module 'collections.abc' has no attribute 'defaultdict'

…that's also visible in the (currently disabled) tests. I need to take a look.

@flokli flokli force-pushed the sphinxcontrib-openapi branch from 475bd57 to 84299d3 Compare August 25, 2022 13:32
@flokli
Copy link
Contributor Author

flokli commented Aug 25, 2022

I got it to work! sphinx now is able to render openapi specs again.

Unfortunately, I wasn't able to get the test suite to parse, but considering it's also disabled for all other sphinx-related python packages too, it's probably not too much of an issue.

@flokli
Copy link
Contributor Author

flokli commented Aug 25, 2022

This apparently got introduced in cilium/openapi@a1d4fca (so I commented there).

We could probably just pin the commit before that instead of undoing the change in a patch but I'd rather see it fixed in cilium/openapi too.

@flokli
Copy link
Contributor Author

flokli commented Aug 25, 2022

I sent a PR at cilium/openapi#1.

@flokli flokli force-pushed the sphinxcontrib-openapi branch from 84299d3 to fb779b2 Compare August 25, 2022 13:50
@SuperSandro2000
Copy link
Member

The prefix for python is python310.pkgs or python310Packages right now. 3.9 is still being build but not the default anymore and python2 is completely unsupported.

@flokli
Copy link
Contributor Author

flokli commented Aug 25, 2022

The prefix for python is python310.pkgs or python310Packages right now. 3.9 is still being build but not the default anymore and python2 is completely unsupported.

Yes, I used the python39 prefix in the PR to convey this unbreaks at least the python 3.9 version of the package - meaning if you assemble a python 3.9 environment with sphinx and this PR, you can render docs. Of course, once ikalnytskyi/picobox#55 is fixed, we should relax the version bounds in picobox.

Until the tests pass, I don't want to mark picocom as not broken on 3.10.

@flokli flokli force-pushed the sphinxcontrib-openapi branch 2 times, most recently from 9ed219d to 4474c8e Compare August 26, 2022 10:02
@flokli
Copy link
Contributor Author

flokli commented Aug 26, 2022

Apparently, the python 3.10 fix for picobox is already in master, there just hasn't been a release yet. I fetchpatch'ed the patch and removed the python 3.9 restriction.

Likewise, sphinx-mdinclude docutils support was fixed too, so no need to ship a downstream patch in nixpkgs, we can fetchpatch this commit too.

In both cases, I linked to the issue/comment asking for a release containing the fixes.

I'd like to wait until cilium/openapi#1 is merged before merging this PR in, so we can point to the final commit there, too.

@flokli flokli force-pushed the sphinxcontrib-openapi branch 2 times, most recently from eadc896 to 62867eb Compare August 26, 2022 11:05
@flokli
Copy link
Contributor Author

flokli commented Aug 26, 2022

cilium/openapi#1 has been merged, this PR was updated. This is ready from my side.

@flokli flokli requested a review from SuperSandro2000 August 26, 2022 11:05
@flokli flokli changed the title python39.pkgs.sphinxcontrib-openapi: unbreak, switch to cilium fork python3.pkgs.sphinxcontrib-openapi: unbreak, switch to cilium fork Aug 26, 2022
flokli added 3 commits August 27, 2022 10:33
This adds a dependency needed for recent sphinxcontrib-openapi.
This fork uses m2r instead of mdinclude.

m2r is unmaintained and incompatible with recent versions of mistune.

See

 - cilium/cilium@b986246
 - cilium/openapi@cd829a0

for details.
@flokli flokli force-pushed the sphinxcontrib-openapi branch from 62867eb to 4c9c4cf Compare August 27, 2022 08:35
@flokli
Copy link
Contributor Author

flokli commented Aug 27, 2022

sphinx-mdinclude did just release 0.5.2, containing the patch, so I updated the version to this and dropped the patch.

@flokli flokli merged commit 1a63545 into NixOS:master Aug 30, 2022
@flokli flokli deleted the sphinxcontrib-openapi branch August 30, 2022 09:06
pname = "picobox";
version = "2.2.0";

disabled = isPy27;
Copy link
Member

Choose a reason for hiding this comment

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

This should be pythonOlder "3.7"

@qmonnet
Copy link

qmonnet commented Jan 16, 2023

[Follow-up] @flokli Hi, the PR to upstream the changes from the cilium fork has been merged a few weeks ago (sphinx-contrib/openapi#127), and a new tag has been created for sphinx/openapi (https://github.com/sphinx-contrib/openapi/releases/tag/0.8.0), so you probably want to revert to that source. We probably won't keep the cilium/ fork forever.

@flokli
Copy link
Contributor Author

flokli commented Jan 16, 2023

@qmonnet thanks for the highlight. I opened #211156, which switches it back to the fork.

flokli added a commit that referenced this pull request Jan 17, 2023
Upstream incorporated the changes present in the cilium fork into their
repository, and tagged a new release (0.8.0). We can switch back to the
upstream version, rather than the temporary fork.

See #188280 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants