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.ruamel-base: add pythonNamespaces to remove nspkg.pth file #250830

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

KiruyaMomochi
Copy link
Contributor

@KiruyaMomochi KiruyaMomochi commented Aug 22, 2023

Description of changes

The Python package ruamel-* uses legacy namespace which produces ruamel.*-1.0.0-py3.10-nspkg.pth file. This causes wrapped python applications importting ruamel.yaml throw a ModuleNotFoundError when $PYTHONPATH is also set to contain ruamel packages. Although I don't understand the cause, this maybe related to
pypa/setuptools#3991.

For example, in a simple devShell with only ansible-lint package, running ansible-lint command gives ModuleNotFoundError: No module named 'ruamel.yaml'. You can try it by running nix develop github:KiruyaMomochi/nix-ruamel-lab and then run ansible-lint.

This issue does not occurs with ruamel-yaml 0.17.21 (update commit), because both the ruamel-base and ruamel-yaml contains .pth file. The former .pth file is overridden by the latter.

This PR fixes the problem by adding pythonNamespaces to all ruamel-* packages. This triggers pythonNamespacesHook, so that all the .pth files are removed in these packages.

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/)
  • 23.11 Release Notes (or backporting 23.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
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@tjni tjni left a comment

Choose a reason for hiding this comment

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

Thank you so much for doing this! I had no idea that the pythonNamespaces option does exactly what is needed here. Tangentially, it will make updating Sphinx so much easier.

Only ruamel-base needs this, which you can verify by looking inside the sitePackages directory of the built derivation. ruamel-yaml was updated to use implicit namespaces, and ruamel-yaml-clib doesn't seem to have a namespace inside of its output.

@tjni tjni mentioned this pull request Aug 22, 2023
12 tasks
@KiruyaMomochi KiruyaMomochi changed the title python3.pkgs.ruamel-*: add pythonNamespaces to remove nspkg.pth file python3.pkgs.ruamel-base: add pythonNamespaces to remove nspkg.pth file Aug 23, 2023
@KiruyaMomochi
Copy link
Contributor Author

Thanks. I have removed pythonNamespaces in ruamel-{yaml,yaml-clib} packages.

This hook seems a little hard to discover, and I found it only by searching nspkg.pth in the full nixpkgs repository. Maybe we should mention it in nixpkgs or wiki? I can create another PR for it.

@tjni
Copy link
Contributor

tjni commented Aug 23, 2023

  1. Would it be possible given the number of rebuilds to rebase this against staging? It's the policy to help manage risk and alleviate the pressure on the build infrastructure (and it does mean it will take a week or two for the change to make it back into unstable).

  2. Adding a section about it to the documentation would be great! I can see perhaps underneath the "Setup hooks" section in the Python language section for now.

I verified that this fixes the build of pre-commit-hook-ensure-sops, which was the only one I knew currently that was affected by this issue on Hydra.

Copy link
Contributor

@tjni tjni left a comment

Choose a reason for hiding this comment

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

Thank you!

@tjni tjni merged commit a629411 into NixOS:staging Aug 23, 2023
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 8.has: package (new) This PR adds a new package labels Aug 23, 2023
@ghost
Copy link

ghost commented Aug 23, 2023

@KiruyaMomochi, thanks for submitting this PR!

In the future, when changing the base branch, I recommend first marking the PR as draft. Then change the branch and force-push, then unmark it as draft.

Github's UI for changing the base branch is really terrible, and often causes a mass-review-request from everybody in the codeowners file during the branch change. But it won't do this for a PR which is marked as draft at the time the base-change or force-push happens.

@KiruyaMomochi KiruyaMomochi deleted the ruamel-base branch August 24, 2023 00:28
@wfdewith wfdewith mentioned this pull request Aug 28, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants