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

rerun: 0.13.0 -> 0.18.2 #334854

Merged
merged 1 commit into from
Sep 15, 2024
Merged

rerun: 0.13.0 -> 0.18.2 #334854

merged 1 commit into from
Sep 15, 2024

Conversation

RobWalt
Copy link
Contributor

@RobWalt RobWalt commented Aug 15, 2024

Description of changes

  • the patches are not necessary anymore
  • the repository now exposes other build targets, so we need to specifically build the rerun-cli package with native_viewer feature
  • this crate was also hit by this issue with rust 1.80, the crate itself ships with a rust-toolchain file which states which rust version they use upstream, so I went ahead and build the crate with a non standard rustPlatform (1.76 instead of 1.80 currently, but the toolchain now updates with the src)

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.

@RobWalt RobWalt force-pushed the rerun-0.17 branch 2 times, most recently from d9285e9 to 4fc9a79 Compare August 15, 2024 17:02
pkgs/by-name/re/rerun/package.nix Outdated Show resolved Hide resolved

# patch https://github.com/NixOS/nixpkgs/issues/332957
postPatch = ''
ln -s ${./Cargo.lock} Cargo.lock
Copy link
Member

Choose a reason for hiding this comment

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

Upstream provides a Cargo.lock, so we don't need to put it in Nixpkgs — we can use cargoHash and just put a small patch updating time in cargoPatches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, TIL cargoPatches exist! I didn't know about that yet. Are there any better docs on the the rust utilities other than this or grepping through nixpkgs?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so. The documentation you linked does mention cargoPatches, but not in a reference list of attributes, so I can see how you missed it. I've found that using grep to try to find how the same problem is solved in other packages to be the best way.

Copy link
Contributor

Choose a reason for hiding this comment

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

@RobWalt you beat me to it! Thanks!

I noticed the patch generated from the master branch isn't applicable to the release commit, so I wanted to look into whether it's possible to pass a postPatch to the FOD and use some kind of a TOML editor to reset the versions unconditionally so that the "patch" would be more generally applicable. I'm unhappy about override-ability of this kind of packages...

@alyssais
Copy link
Member

   error: undefined variable 'RobWalt'

Do you need to add yourself to maintainers.nix? (If so, please do so in a separate commit, before the current one.)

@RobWalt
Copy link
Contributor Author

RobWalt commented Aug 16, 2024

   error: undefined variable 'RobWalt'

Do you need to add yourself to maintainers.nix? (If so, please do so in a separate commit, before the current one.)

I'm sorry for all the little nits you needed to comment on. It was just robwalt instead of RobWalt 🤦🏼

@ofborg ofborg bot requested a review from SomeoneSerge August 16, 2024 14:59
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Aug 16, 2024
@SomeoneSerge
Copy link
Contributor

SomeoneSerge commented Aug 16, 2024

@ofborg build python312Packages.rerun-sdk

Did I forget to add that to passthru tests?

@SomeoneSerge
Copy link
Contributor

rerun-cli: 0.13.0 -> 0.17.0

The name of the package in nixpkgs is rerun, that should reflect in the commit message in order to trigger Ofborg. It seems like upstream has changed the target name in their cargo file? I'm not opposed to renaming the attribute to rerun-cli instead of rerun (with an alias), although I feel like that'd be unnecessary friction

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you also need to patch Cargo.toml and its rust-version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is mostly cosmetric and not picked up by cargo commands

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently not? Ofborg says:

       >
       > Cargo.lock is not the same in /build/rerun-0.17.0-vendor.tar.gz
       >
       > To fix the issue:
       > 1. Set cargoHash/cargoSha256 to an empty string: `cargoHash = "";`
       > 2. Build the derivation and wait for it to fail with a hash mismatch
       > 3. Copy the "got: sha256-..." value back into the cargoHash field
       >    You should have: cargoHash = "sha256-XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX

https://github.com/NixOS/nixpkgs/pull/334854/checks?check_run_id=28868560235

@RobWalt
Copy link
Contributor Author

RobWalt commented Aug 16, 2024

rerun-cli: 0.13.0 -> 0.17.0

The name of the package in nixpkgs is rerun, that should reflect in the commit message in order to trigger Ofborg. It seems like upstream has changed the target name in their cargo file? I'm not opposed to renaming the attribute to rerun-cli instead of rerun (with an alias), although I feel like that'd be unnecessary friction

I think in the end it's just the package name that changed and is now called rerun-cli. The binary is still named rerun. I'll change the title of the PR

@RobWalt RobWalt changed the title rerun-cli: 0.13.0 -> 0.17.0 rerun: 0.13.0 -> 0.17.0 Aug 16, 2024
@SomeoneSerge
Copy link
Contributor

I'll change the title of the PR

ok by me but Ofborg looks at the commit message not at the PR title

@RobWalt
Copy link
Contributor Author

RobWalt commented Aug 19, 2024

@SomeoneSerge I think it ran through now successfully 🤠

Copy link
Contributor

Choose a reason for hiding this comment

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

H'm, I'm still seeing this:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, you're right 😬

@alyssais
Copy link
Member

alyssais commented Sep 7, 2024

Hash mismatch on OfBorg.

@SomeoneSerge
Copy link
Contributor

Yes, my presumption still is that it's #334854 (comment)

@RobWalt RobWalt force-pushed the rerun-0.17 branch 3 times, most recently from 2403d98 to 27a81e3 Compare September 12, 2024 09:15
@RobWalt RobWalt changed the title rerun: 0.13.0 -> 0.17.0 rerun: 0.13.0 -> 0.18.2 Sep 12, 2024
@RobWalt
Copy link
Contributor Author

RobWalt commented Sep 12, 2024

Took me a while but the passthru tests now build locally and are all green. Thanks to you two I learned a lot!

@SomeoneSerge FYI

Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

I'll leave the final check to @SomeoneSerge.

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

I didn't do much thorough testing but nix run github:RobWalt/nixpkgs/rerun-0.17#rerun works as expected. Thank you @RobWalt!

@SomeoneSerge SomeoneSerge merged commit cd16cb5 into NixOS:master Sep 15, 2024
27 checks passed
@RobWalt RobWalt deleted the rerun-0.17 branch September 15, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: python 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants