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

python311Packages.ffcv: fix tests #322042

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Conversation

djacu
Copy link
Member

@djacu djacu commented Jun 23, 2024

Description of changes

Fixes python3Packages.ffcv tests by changing the directory in precheck.

ffcv was removed from python312Packages because it does not successfully build due to a dependency on distutils. Looks like they have a version 1.1.0 branch in the works that removes that but it has not been released yet.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • 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.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/packaging-a-python-library-with-c-distutils-extension/17732/5

@djacu
Copy link
Member Author

djacu commented Jun 23, 2024

Result of nixpkgs-review pr 322042 run on x86_64-linux 1

2 packages built:
  • python311Packages.ffcv
  • python311Packages.ffcv.dist

Copy link
Member

@samuela samuela left a comment

Choose a reason for hiding this comment

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

thanks for putting this together @djacu !

pkgs/development/python-modules/ffcv/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/ffcv/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/ffcv/default.nix Outdated Show resolved Hide resolved
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jun 24, 2024
@djacu djacu force-pushed the fix-ffcv-python branch from 27f6c33 to 01d5ea8 Compare June 25, 2024 00:09
@djacu
Copy link
Member Author

djacu commented Jun 25, 2024

@samuela @SomeoneSerge Thanks for the reviews and feedback. I have made changes or responded to your comments if the changes were not possible. I've left some of the comments unresolved so you can see what I did and make sure it is satisfactory.

@djacu
Copy link
Member Author

djacu commented Jun 25, 2024

Result of nixpkgs-review pr 322042 run on x86_64-linux 1

2 packages built:
  • python311Packages.ffcv
  • python311Packages.ffcv.dist

@ofborg ofborg bot requested a review from samuela June 25, 2024 00:39
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jun 25, 2024
@djacu djacu force-pushed the fix-ffcv-python branch from 01d5ea8 to 85e8a72 Compare June 26, 2024 03:43
@djacu djacu requested a review from SomeoneSerge June 26, 2024 03:44
@djacu
Copy link
Member Author

djacu commented Jun 26, 2024

@samuela @SomeoneSerge all comments addressed

@SomeoneSerge SomeoneSerge changed the title python3Packages.ffcv: fix tests python311Packages.ffcv: fix tests Jun 26, 2024
@SomeoneSerge
Copy link
Contributor

SomeoneSerge commented Jun 26, 2024

Ah, I guess it's the commit names that ofborg looks at, not PR name; @djacu could you please update the commit message as per https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions? (if you browse through the "show all checks" you'll see that ofborg evaluated all outPaths but it skipped building ffcv

Fixes python3Packages.ffcv tests by changing the directory in precheck.
Cleaned up and refactored the derivation.
@djacu djacu force-pushed the fix-ffcv-python branch from 85e8a72 to 968cbb1 Compare June 26, 2024 15:03
@djacu
Copy link
Member Author

djacu commented Jun 26, 2024

@SomeoneSerge TIL. I thought it was the PR title as well.

I didn't see explicit guidance in that doc but it links to other documents about how the commit message should look. Like this one -> https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#commit-conventions
I think I changed it to something that will trigger ofborg.

Also, we should be able to trigger it manually but I agree that getting the commit message consistent with the contributing guide would be best. https://github.com/NixOS/ofborg?tab=readme-ov-file#build

@SomeoneSerge
Copy link
Contributor

For python packages it ofborg does want the minor version of the package set specified too (python311Packages.ffcv), which I only recently discovered. But this is now good enough for merging

@ofborg build python3Packages.ffcv

@djacu
Copy link
Member Author

djacu commented Jun 26, 2024

Hmm the guide is not clear what to do in cases other than init or version bumps. Is the init example a very specific example of a more generic capability? For example, the guide says for initial commits to write the commit message as <package>: init at <version>, but is a more generic message like <package>: <action> at <version> possible? So writing python3Packages.ffcv: bugfixes at 1.0.0 or python3Packages.ffcv: fix tests at 1.0.0 are valid?

@djacu
Copy link
Member Author

djacu commented Jun 26, 2024

Result of nixpkgs-review pr 322042 run on x86_64-linux 1

2 packages built:
  • python311Packages.ffcv
  • python311Packages.ffcv.dist

@djacu
Copy link
Member Author

djacu commented Jun 26, 2024

Looks like the aarch64-darwin build on master has the same issue.
https://hydra.nixos.org/build/264481981

And x86-darwin has it's own issues.
https://hydra.nixos.org/build/264482265

I'm wondering how/why the darwin builds run the tests when the linux builds don't.
https://hydra.nixos.org/build/263558079/log
https://hydra.nixos.org/build/264482315/log

@SomeoneSerge SomeoneSerge merged commit 403459a into NixOS:master Jun 26, 2024
23 of 24 checks passed
@djacu djacu deleted the fix-ffcv-python branch June 26, 2024 23:57
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.

5 participants