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

ledger-autosync: fix build with Python 3.12 and don't propagate ledger and hledger #334202

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

dotlambda
Copy link
Member

Description of changes

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/)
  • 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.

@emilazy
Copy link
Member

emilazy commented Aug 12, 2024

What do we need 3.11 for here? Upstream doesn’t actually mention nose at all, so I was hoping we could keep this running on 3.12 and move it over to pytestCheckHook.

@dotlambda
Copy link
Member Author

dotlambda commented Aug 12, 2024

What do we need 3.11 for here?

for imp: https://github.com/egh/ledger-autosync/blob/v1.0.3/ledgerautosync/cli.py#L24

@dotlambda
Copy link
Member Author

Upstream doesn’t actually mention nose at all

True. I dropped the nose dependency.

@emilazy
Copy link
Member

emilazy commented Aug 12, 2024

Sorry, should have read more closely…

Seems reasonable (albeit unfortunate) to me, but since upstream uses pytest, can we at least drop the nose use here?

Edit: Ah, you’re ahead of me!

@dotlambda
Copy link
Member Author

The alternative to using python311Packages would be applying egh/ledger-autosync@b7a2a18 and egh/ledger-autosync@453d92a.

@emilazy
Copy link
Member

emilazy commented Aug 12, 2024

Perhaps we should just bump to the current Git HEAD. The conversion to pytest was actually after the latest release, and upstream was planning on cutting a release in July. That hasn’t yet happened, but there’s been a lot of clean‐up and fixes since, so if upstream thought it was ready for release I say we should just bump.

(I was looking at these packages a while ago but paged out most of the memories…)

@dotlambda dotlambda changed the title ledger-autosync: use python311Packages and don't propagate ledger and hledger ledger-autosync: fix build with Python 3.12 and don't propagate ledger and hledger Aug 12, 2024
@DamienCassou
Copy link
Contributor

Perhaps we should just bump to the current Git HEAD

I think this makes sense given the state of the project. I'm fine merging the PR as it is though.

What do you say @dotlambda?

@dotlambda
Copy link
Member Author

dotlambda commented Aug 12, 2024

Let's give upstream a little more time to cut a release before we use the git HEAD.

Copy link
Contributor

@DamienCassou DamienCassou left a comment

Choose a reason for hiding this comment

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

Changes are great, thank you for your work. I'm fine merging that or waiting for upstream to release or using git HEAD.

@dotlambda dotlambda merged commit bd909c8 into NixOS:master Aug 13, 2024
29 of 30 checks passed
@dotlambda dotlambda deleted the ledger-autosync branch August 13, 2024 07:58
@emilazy emilazy mentioned this pull request Aug 22, 2024
13 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.

3 participants