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

Re-add grab-site with updates to dependencies for python 3.12 #321601

Closed
wants to merge 14 commits into from

Conversation

pastly
Copy link

@pastly pastly commented Jun 21, 2024

Description of changes

Re-adds and bumps the version of ludios_wpull as necessary for grab-site to work with python 3.12. Consequently bumps tornado to 6.4 and adds packaging as a dependency to yapsy (like it's supposed to be!).

The ludios_wpull version update sounds huge, but it's just one real bump. No changelog in the project, but see release notes.

Tornado release notes page. It's only a bump of two versions. I stopped at 6.4.0 instead of going to 6.4.1 so that the nix mitmproxy tests wouldn't barf at a tornado version greater than 6.4 (I was running nix-build -A pkgs.python3Packages.tornado.passthru.tests as docs suggested`)

I'm running the above passthru.tests command now ... but it's seemingly going to take a long time since tornado is a popular library.

Do I need to run it myself? Do I need to run it for more versions of python?

Thanks

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true (it's Linux)
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

}:

buildPythonPackage {
with python.pkgs; buildPythonPackage {
Copy link
Member

Choose a reason for hiding this comment

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

can you explain the reason for this change ?

Copy link
Author

@pastly pastly Jun 22, 2024

Choose a reason for hiding this comment

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

Its packaging dependency needs it; In an upcoming commit I've removed the with python.pkgs and instead used python.pkgs.packaging as the dependency. Thanks.

pastly added a commit to pastly/nixpkgs that referenced this pull request Jun 22, 2024
@pastly
Copy link
Author

pastly commented Jun 22, 2024

Oh, I should probably cc @ivan as they were the original maintainer of these packages, I'm re-adding them after their removal, and as the are now, I didn't list a maintainer.

Would you like me to add you back?

@ivan
Copy link
Member

ivan commented Jun 22, 2024

Yes, I would like to be in the maintainers, thanks.

defaultOverrides = [
# override the version of packages pinned in pyproject.toml
(self: super: {
sqlalchemy = super.sqlalchemy.overridePythonAttrs (oldAttrs: rec {
Copy link
Member

Choose a reason for hiding this comment

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

overrides can not be done for python modules, because it would introduce clashes with other dependencies

overrides can be done for applications. from my initial look wpull is intended to be used as an application and not python module ( eg import )

if that's the case then it needs to be moved to by-name https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/README.md

Copy link
Author

Choose a reason for hiding this comment

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

Moved, thanks. And thank for your patience.

@@ -0,0 +1,47 @@
{ lib
, python312
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I believe I have addressed this one correctly. It's the main thing I doubt myself on regarding your latest feedback.

I didn't do it like your link instructs b/c it seemed like other packages in all-packages.nix with the same problem were using python312Packages.callPackage to solve it.

Copy link
Author

Choose a reason for hiding this comment

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

... okay that didn't work. I've done it like these docs say. I finally figured out the right combo of python___ stuff.

pastly added a commit to pastly/nixpkgs that referenced this pull request Jun 25, 2024
pastly added a commit to pastly/nixpkgs that referenced this pull request Jun 25, 2024
pastly added a commit to pastly/nixpkgs that referenced this pull request Jun 25, 2024
@ofborg ofborg bot requested a review from ivan June 25, 2024 01:42
@kirillrdy
Copy link
Member

@pastly so currently this PR causes a lot of rebuilds, which means it should target staging branch.

staging already has tornado 6.4.1, so you can drop this patch

to read more about branches workflow https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#staging

please note a gotcha when changing target branch for a PR https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#rebasing-between-branches-ie-from-master-to-staging

@pastly pastly marked this pull request as draft June 30, 2024 00:07
pastly added 2 commits June 29, 2024 19:31
Upstream's[0] requirements.txt files don't list it as a dependency, but
it totally is one[1].

I hit a runtime error when trying to package ludios_wpull, which
itself addresses[2] yapsy's dependency on packaging.

0: https://github.com/tibonihoo/yapsy
1: https://github.com/tibonihoo/yapsy/blob/6b487b04affb19ab40adbbc87827668bea0abcee/package/yapsy/PluginInfo.py#L16
2: https://github.com/ArchiveTeam/ludios_wpull/blob/f6cf1c3375d6722a742aa7e3dc463e528eaec906/pyproject.toml#L31
@pastly pastly force-pushed the grab-site-and-reqs branch from 70f0dc3 to d147bd7 Compare June 30, 2024 00:32
@pastly pastly changed the base branch from master to staging June 30, 2024 00:32
@pastly pastly marked this pull request as ready for review June 30, 2024 00:35
@kirillrdy
Copy link
Member

tornado 6.4.1 is now on master, i recommend to target master for this PR

don't forget the gotchata with changing target branches

@pastly pastly closed this Jul 11, 2024
@ivan
Copy link
Member

ivan commented Jul 15, 2024

@pastly thanks for working on this, do you want someone else to take this up, or was it accidentally closed, or did you intend to make a new PR for master?

@pastly
Copy link
Author

pastly commented Jul 29, 2024

@pastly thanks for working on this, do you want someone else to take this up, or was it accidentally closed, or did you intend to make a new PR for master?

The process was discouraging and I've temporarily/permanently tabled the project I was working on using these packages.

No plans to make a PR for master, but anyone else is welcome to do so.

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.

4 participants