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

Add override for Pendulum version 3.0.0 with Rust #1739

Merged
merged 12 commits into from
Sep 29, 2024

Conversation

bow
Copy link
Contributor

@bow bow commented Jul 16, 2024

Hi maintainers,

I recently came across build problems when trying to use poetry2nix on a Python project that depends on Pendulum. The build error was because this was the first Pendulum release that has parts of its source re-written in Rust. Previous versions have been pure Python.

This PR fixes that issue by adding an override for Pendulum that includes the Rust build hook. The override is only applied if the version is at least 3.0.0 (still the most recent version at the time of writing).

The changeset of this PR works locally and I have also included a test for this, to confirm also that builds of previous versions are not broken. That said, I did encounter an issue where I found out the version attribute of the autogenerated Cargo.lock in pendulum's source tree is actually 3.0.0-beta-1. This introduced some conflict when building.

Furthermore, I found out that the commit on master on which the GitHub release was done is actually different from the 3.0.0-tagged commit. One of the difference being, that the [project].version value in pyproject.toml of those two commits differ (3.0.0 vs 3.0.0b1). I suspect this was the cause of the differing version value in the Cargo.lock file.

These differences, as noted in the code, led me to the use of fetchCargoTarball instead of importCargoLock. The latter would require some manual string replacemend (e.g. in prePatch phase), which I think complicates the override too much.

EDIT: Apparently fetchCargoTarball still produces the same error (hash mismatch). My local builds were passing, but it seemed to be caused by some intermediate derivation not required for the final build. CI says otherwise, and when I run garbage collection, I bumped on the same error again. So now, I am using importCargoLock with an explicit Cargo.lock file added as part of this MR. Turns out this simplifies the build, in that we do not need to download and patch the source tree anymore.

Anyway, I hope this is useful ~ and feel free to critique, this is my first contribution to the repo 🙂.

Contribution checklist (recommended but not always applicable/required):

  • There's an automated test for this change
  • Commit messages or code include references to related issues or PRs (including third parties)
  • Commit messages are conventional - examples from the log include "feat: add changelog files to fixup hook", "fix(contourpy): allow wheel usage", and "test: add sqlalchemy2 test"

@bow
Copy link
Contributor Author

bow commented Jul 16, 2024

An additional question not directly related to this PR: During the course of playing with this package (that depends on pendulum), I found several other Python packages that needed setuptools as their build dependency.

I suspect the fix for these is just new entries in overrides/build-systems.json.

So for these packages, do you prefer a new PR altogether or is it ok to just add commits in this PR?

bow added a commit to bow/poetry2nix that referenced this pull request Jul 16, 2024
Previous approach of patching the source prior to build always results
in some hash mismatch. So here we just use a drop-in Cargo.lock file,
or rather what it would have looked like had the version been set
correctly.

See: nix-community#1739
overrides/default.nix Outdated Show resolved Hide resolved
@cpcloud cpcloud merged commit ecb1843 into nix-community:master Sep 29, 2024
181 checks passed
cheriimoya pushed a commit to cheriimoya/poetry2nix that referenced this pull request Oct 2, 2024
cheriimoya pushed a commit to cheriimoya/poetry2nix that referenced this pull request Oct 2, 2024
cheriimoya pushed a commit to cheriimoya/poetry2nix that referenced this pull request Oct 2, 2024
cheriimoya pushed a commit to cheriimoya/poetry2nix that referenced this pull request Oct 2, 2024
cheriimoya pushed a commit to cheriimoya/poetry2nix that referenced this pull request Oct 2, 2024
Previous approach of patching the source prior to build always results
in some hash mismatch. So here we just use a drop-in Cargo.lock file,
or rather what it would have looked like had the version been set
correctly.

See: nix-community#1739
Arkptz pushed a commit to Arkptz/poetry2nix that referenced this pull request Nov 2, 2024
Arkptz pushed a commit to Arkptz/poetry2nix that referenced this pull request Nov 2, 2024
Arkptz pushed a commit to Arkptz/poetry2nix that referenced this pull request Nov 2, 2024
Previous approach of patching the source prior to build always results
in some hash mismatch. So here we just use a drop-in Cargo.lock file,
or rather what it would have looked like had the version been set
correctly.

See: nix-community#1739
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants