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

python312Packages.{webassets, flask-assets}: remove nose; modernize #348621

Merged
merged 5 commits into from
Oct 15, 2024

Conversation

pyrox0
Copy link
Member

@pyrox0 pyrox0 commented Oct 14, 2024

Follows up and includes the work from #334483. Fixes some broken subsitutions in flask-assets and enables some tests in webassets.

Part of #326513.

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.

@pyrox0
Copy link
Member Author

pyrox0 commented Oct 14, 2024

Currently running nixpkgs-review, will add output of that below once I've finished running it.

@pyrox0
Copy link
Member Author

pyrox0 commented Oct 14, 2024

Getting some build errors, trying to isolate the issue now.

Edit: Fixed some dukpy test errors. Investigating some flask-mail test errors, which is weird because I don't think it's got a dependency on anything here that's been changed.

@pyrox0
Copy link
Member Author

pyrox0 commented Oct 14, 2024

Confirmed that the flask-mail test errors are unrelated to this PR. Pushing my dukpy changes, this is ready to review.

Comment on lines +24 to +28
postPatch = ''
substituteInPlace tests/test_webassets_filter.py \
--replace-fail "class PyTestTemp" "class _Temp" \
--replace-fail "PyTestTemp" "Temp"
'';
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what’s going on here? Was it broken already or do the changes in this PR somehow make this necessary?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, it’s adapting for the changes in webassets.test. Okay, that seems fine then; hopefully upstream will release and the ecosystem will adapt at some point so that we no longer need to carry this.

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

It’s a shame to have to carry a patch like this around but there’s not much we can do when upstream hasn’t properly finished the migration and it’s too widely depended upon to drop. (And anyway, that’s inherited from the previous PR.)

We’re almost at the finish line!

Result of nixpkgs-review pr 348621 run on aarch64-linux 1

1 package failed to build:
  • powerdns-admin
14 packages built:
  • pyload-ng
  • pyload-ng.dist
  • python311Packages.dukpy
  • python311Packages.dukpy.dist
  • python311Packages.flask-assets
  • python311Packages.flask-assets.dist
  • python311Packages.webassets
  • python311Packages.webassets.dist
  • python312Packages.dukpy
  • python312Packages.dukpy.dist
  • python312Packages.flask-assets
  • python312Packages.flask-assets.dist
  • python312Packages.webassets
  • python312Packages.webassets.dist

Comment on lines +24 to +28
postPatch = ''
substituteInPlace tests/test_webassets_filter.py \
--replace-fail "class PyTestTemp" "class _Temp" \
--replace-fail "PyTestTemp" "Temp"
'';
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, it’s adapting for the changes in webassets.test. Okay, that seems fine then; hopefully upstream will release and the ecosystem will adapt at some point so that we no longer need to carry this.

@emilazy emilazy merged commit df70905 into NixOS:master Oct 15, 2024
29 of 30 checks passed
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