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

archivebox: fix build and add singlefile #358588

Merged
merged 2 commits into from
Dec 2, 2024

Conversation

james-atkins
Copy link
Contributor

@james-atkins james-atkins commented Nov 23, 2024

Fixes #358580.

Override any django-extensions patches.
Downgrade to Python 3.11 as django needs distutils.

Avoid a scoping error with yt-dlp. If pkgs.yt-dlp is in dependencies instead of python.pkgs.yt-dlp, there is
a cryptic error about 'pdm.backend' not being available. Change to just using python.pkgs.yt-dlp everywhere.

Also update buildPythonApplication to use build-system and dependencies.

Add SingleFile support too.

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
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Why not remove yt-dlp from the inputs instead?
Also, the commit message doesn't make sense since we're merging this into master first.

@james-atkins james-atkins force-pushed the pr/fix-archivebox-24.11 branch from e61b23c to 5a426ea Compare November 24, 2024 15:14
@james-atkins
Copy link
Contributor Author

Fixed the commit message. We need pkgs.yt-dlp for the environment variable later in the code so we can't remove it.

@james-atkins james-atkins changed the title archivebox: fix build on NixOS 24.11 archivebox: fix build Nov 24, 2024
@dotlambda
Copy link
Member

We need pkgs.yt-dlp for the environment variable later in the code so we can't remove it.

Use python.pkgs.yt-dlp for that.

Fixes NixOS#358580.

Override any django-extensions patches.
Add distutils dependency for Python 3.12 and above.
Convert buildPythonApplication to use build-system and dependencies.
Fix a scoping error with yt-dlp.

Note: if neither python.pkgs.yt-dlp nor pkgs.yt-dlp are in archivebox's
dependencies, then there is a missing runtime dependency error.
If pkgs.yt-dlp is in dependencies instead of python.pkgs.yt-dlp, (as was
the case) there is a cryptic error about 'pdm.backend' not being available.
Fix this by just using `python.pkgs.yt-dlp` everywhere.
@james-atkins james-atkins force-pushed the pr/fix-archivebox-24.11 branch from 5a426ea to 018bd5d Compare November 24, 2024 15:39
@james-atkins james-atkins changed the title archivebox: fix build archivebox: fix build and add singlefile Nov 24, 2024
@siraben
Copy link
Member

siraben commented Dec 2, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 358588


aarch64-darwin

⏩ 2 packages marked as broken and skipped:
  • archivebox
  • archivebox.dist

@dotlambda dotlambda added the backport release-24.11 Backport PR automatically label Dec 2, 2024
@dotlambda
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 358588


x86_64-linux

✅ 2 packages built:
  • archivebox
  • archivebox.dist

@@ -34,6 +34,7 @@ let
"CVE-2022-28346"
];
};
dependencies = (old.dependencies or [ ]) ++ (lib.optionals (python.pythonAtLeast "3.12") [ python.pkgs.distutils ]);
Copy link
Member

Choose a reason for hiding this comment

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

Why is distutils in dependencies rather than build-system?

Copy link
Contributor Author

@james-atkins james-atkins Dec 2, 2024

Choose a reason for hiding this comment

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

This version of django uses distutils for parsing version strings. https://github.com/django/django/blob/stable/3.1.x/django/utils/version.py#L6

@@ -34,6 +34,7 @@ let
"CVE-2022-28346"
];
};
dependencies = (old.dependencies or [ ]) ++ (lib.optionals (python.pythonAtLeast "3.12") [ python.pkgs.distutils ]);
Copy link
Member

@dotlambda dotlambda Dec 2, 2024

Choose a reason for hiding this comment

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

Suggested change
dependencies = (old.dependencies or [ ]) ++ (lib.optionals (python.pythonAtLeast "3.12") [ python.pkgs.distutils ]);
build-system = (old.build-system or [ ]) ++ (lib.optionals (self.pythonAtLeast "3.12") [ self.distutils ]);

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 2, 2024
@dotlambda dotlambda merged commit 049271b into NixOS:master Dec 2, 2024
54 of 55 checks passed
Copy link
Contributor

github-actions bot commented Dec 2, 2024

Successfully created backport PR for release-24.11:

@james-atkins james-atkins deleted the pr/fix-archivebox-24.11 branch December 9, 2024 20:53
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.

Build failure: archivebox on NixOS 24.11
4 participants