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

spades: 3.15.5 -> 4.0.0 #320502

Merged
merged 1 commit into from
Jul 3, 2024
Merged

spades: 3.15.5 -> 4.0.0 #320502

merged 1 commit into from
Jul 3, 2024

Conversation

bzizou
Copy link
Contributor

@bzizou bzizou commented Jun 17, 2024

Description of changes

Upgrade to Spades 4.0.0

  • compilation bug with gcc 13 has been fixed upstream
  • copytree.patch suggested and accepted upstream

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/) ---> ./result/bin/spades.py --test
  • 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.

@superherointj
Copy link
Contributor

@bzizou I want to mark this as draft until you are ready for review.

Possibilities:

  1. Use fetchpatch now. (pinned to commit)
  2. Wait for upstream to catch up with patch.

@bzizou bzizou force-pushed the spades4 branch 2 times, most recently from 32832a5 to cfcd405 Compare July 1, 2024 13:38
@superherointj
Copy link
Contributor

superherointj commented Jul 1, 2024

Can you add:

meta.changelog = "https://github.com/ablab/spades/blob/v${finalAttrs.version}/changelog.md";
meta.downloadPage = "https://github.com/ablab/spades";

@bzizou bzizou force-pushed the spades4 branch 2 times, most recently from 668c46a to c6fcefb Compare July 1, 2024 13:44
@superherointj superherointj marked this pull request as ready for review July 1, 2024 15:14
@superherointj
Copy link
Contributor

Git commit message body (the second line after title) is missing the release notes:

Release: https://github.com/ablab/spades/releases/tag/v4.0.0

@superherointj
Copy link
Contributor

superherointj commented Jul 1, 2024

I have successfully built:

nix build .#pkgsCross.aarch64-multiplatform.spades

Meaning, "aarch64-linux" platform is supported but unlisted.

When adding it, respect alphabetical order.

@superherointj
Copy link
Contributor

superherointj commented Jul 1, 2024

pkgsMusl.spades, pkgsStatic.spades are broken.

nix build .#pkgsMusl.spades

image

Logs: https://termbin.com/sik1

As pkgsMusl.gtest builds fine. Likely problem is the non-Nix vendoring.

A Nix dependency if built independently, will be cached, which de-duplicates builds of this dependency and improve build times for downstream packages. Whenever possible, avoid vendoring and package dependencies as Nix dependencies.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jul 1, 2024
@superherointj
Copy link
Contributor

superherointj commented Jul 2, 2024

Please add:

strictDeps = true;

(This is useful in separating kinds of dependencies for cross build. This should be true by default but for legacy reasons it is not)

Then, you can check if python3 (maybe bzip2?) are only needed during build time, hence nativeBuildInputs. Otherwise if needed for final output it's buildInputs.
Also, check if application work as expected. (To confirm these dependencies are not needed at runtime.)

Copy link
Contributor

@superherointj superherointj left a comment

Choose a reason for hiding this comment

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

fetchpatch of copytree.patch is failing at nixpkgs-review

@bzizou
Copy link
Contributor Author

bzizou commented Jul 2, 2024

I agree that redistributing dependencies is dirty... they are numerous: https://github.com/ablab/spades/tree/next/ext/src

I tried to disable some, to replace them by dependencies to nix package but their Cmake files are not ok, and this leads to includes or libraries not found. My knowledge in cmake is probably not sufficient to find some solutions... and I already spent too much time on this. Sorry. So --> broken = stdenv.hostPlatform.isMusl; Maybe should I also add broken = stdenv.hostPlatform.isStatic; ?

@superherointj
Copy link
Contributor

@bzizou LGTM. Only the Darwin build needs to be confirmed.
By any chance, are you able to build this package for Darwin?
If not, we can ping Darwin maintainers for this.

@superherointj
Copy link
Contributor

@toonn Can you post build results for Darwin? Thanks. Sorry for the inconvenience. CI is just unreliable/slow for Darwin?

Copy link
Contributor

@superherointj superherointj left a comment

Choose a reason for hiding this comment

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

I'm approving this despite Darwin build being unknown.

Update: Watching build at:
https://hydra.nixos.org/build/264908242

@bzizou
Copy link
Contributor Author

bzizou commented Jul 3, 2024

@bzizou LGTM. Only the Darwin build needs to be confirmed. By any chance, are you able to build this package for Darwin? If not, we can ping Darwin maintainers for this.

No, I've got no Darwin host

@superherointj superherointj merged commit ce5429f into NixOS:master Jul 3, 2024
30 checks passed
@superherointj
Copy link
Contributor

Darwin build is broken:

https://hydra.nixos.org/build/264908242/nixlog/1

@bzizou Can it be fixed or should we mark it broken for Darwin?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person awaiting_reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants