Skip to content

Comments

pict-rs_0_3: drop#336077

Merged
emilazy merged 2 commits intoNixOS:masterfrom
emilazy:push-yoykzrxwpykp
Aug 22, 2024
Merged

pict-rs_0_3: drop#336077
emilazy merged 2 commits intoNixOS:masterfrom
emilazy:push-yoykzrxwpykp

Conversation

@emilazy
Copy link
Member

@emilazy emilazy commented Aug 20, 2024

Description of changes

This was broken by the Rust 1.80 upgrade, and is an old version that we’d have to patch to keep working.

We have already done the 0.4 → 0.5 update without keeping around the old version or adding in any additional stateVersion logic in #280221. As a result, migration for 0.3 users is going to be a little awkward. I’ve done my best to provide comprehensive instructions for anyone who hasn’t already bumped to 0.4.

It is probably a footgun to add stateVersion logic for any package that makes backwards‐incompatible schema changes and only supports migration from the immediately previous version. Users won’t get migrated by default and we have to either package and maintain an endlessly growing list of old versions or add complicated instructions like this. It’s not really practical for us to support a significantly better migration story than upstream does.

cc @happysalada

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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 20, 2024
@ofborg ofborg bot added 8.has: clean-up This PR removes packages or removes other cruft 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Aug 20, 2024
@marius851000
Copy link
Contributor

Oh, that’s problematic if it only support update from 0.3 to 0.4 and there is no such 0.4 in nixpkgs. I’ll try to override the package to update my server.

@emilazy
Copy link
Member Author

emilazy commented Aug 20, 2024

Yes, it’s unfortunate, I’m sorry :( We had 0.4 before the release of 24.05, but the bump happened without being mentioned in the release notes AFAIK.

stateVersion was probably never the right thing here since the migration can be done automatically and we can’t really maintain and patch an endlessly‐growing set of versions just to keep rollback working when the upstream migration code doesn’t keep that compatibility. Any users still on 0.3 were in a bad spot already before this PR, but hopefully the migration instructions can help them get out of it.

Ideally upstream would restore the 0.3 migration code to 0.5, but I don’t know how complicated that would be. cc @asonix

@asonix
Copy link
Contributor

asonix commented Aug 20, 2024

My stance on bringing back the 0.3 migration code in 0.5 is that I'd rather not. It's unfortunate that nixpkgs ended up in this situation though.

@marius851000
Copy link
Contributor

I totally forgot I can just mix different version of Nixpkgs to pick a specific package from one. That’ll be quite usefull (as 0.4.7 fail to compile for the same reason than 0.3)

@emilazy
Copy link
Member Author

emilazy commented Aug 20, 2024

My stance on bringing back the 0.3 migration code in 0.5 is that I'd rather not. It's unfortunate that nixpkgs ended up in this situation though.

Alright. I can understand not wanting to keep that legacy code forever, but hopefully you can understand that we also don’t want to (really: can’t, given maintainer resources) do the work of maintaining packages for old versions indefinitely and patching the code ourselves if they break, just for migration purposes. I don’t think there’s any way out here other than to roll forward and make sure we mention the major version bumps in the changelog in future.

I totally forgot I can just mix different version of Nixpkgs to pick a specific package from one. That’ll be quite usefull (as 0.4.7 fail to compile for the same reason than 0.3)

Right, that’s probably the best thing you can do to get the migration done. Did you try following my instructions in the error message I added? I’d appreciate any suggestions on how they could be made clearer, or any issues you run into during the process.

@happysalada
Copy link
Contributor

Looks good to me

@emilazy
Copy link
Member Author

emilazy commented Aug 21, 2024

I’ll wait a day or so to hear back from @marius851000 about the migration and then merge. This can’t make 0.3 more broken than it already is, so it’s only a matter of how good our migration instructions are.

@marius851000
Copy link
Contributor

marius851000 commented Aug 21, 2024

Okay, I’m back (after some issue pushing the build on the server). So, using

services.pict-rs.package = (import (pkgs.fetchFromGitHub {
    owner = "NixOS";
    repo = "nixpkgs";
    rev = "9b19f5e77dd906cb52dade0b7bd280339d2a1f3d";
    sha256 = "sha256-rCIsyE80jgiOU78gCWN3A0wE0tR2GI5nH6MlS+HaaSQ=";
  }) {}).pict-rs;

does work (if you don’t mind Import From Derivation. Otherwise, it could be adapted to use fetchurl).

What didn’t worked was, after the migration for 0.4 was ran and switching to services.pict-rs.package = pkgs.pict-rs; was port binding, as it seems pict-rs 0.5, as configured in NixPkgs, does not respect the services.pict-rs.port port property (and 8080 is already used for... (check config) a custom hacky BOINC account manager).

@marius851000
Copy link
Contributor

Configuration tells me that PICTRS__SERVER__ADDRESS should be used instead of PICTRS__SERVER__ADDR. I’ll test that locally (and if it does work, I’ll suggest fixing it in this PR, even if that’s slightly outside of the scope of this PR)

@emilazy
Copy link
Member Author

emilazy commented Aug 21, 2024

Thanks for trying it out! I’ll happily deal with that fix in this PR if you confirm it works. Do you think it would be a good idea for us to document an exact services.pict-rs.package setting that should work for the transition? Not knowing how to select a package from a pinned Nixpkgs was one of my big worries about these instructions. We should be able to use builtins.fetchTarball to avoid IFD.

@emilazy emilazy mentioned this pull request Aug 21, 2024
13 tasks
@marius851000
Copy link
Contributor

So, you can use this:

services.pict-rs.package = (import (builtins.fetchTarball {
    url = "https://github.com/NixOS/nixpkgs/archive/9b19f5e77dd906cb52dade0b7bd280339d2a1f3d.tar.gz";
    sha256 = "sha256:0939vbhln9d33xkqw63nsk908k03fxihj85zaf70i3il9z42q8mc";
  }) {}).pict-rs;

And PICTRS__SERVER__ADDRESS should indeed be used. It does bind correctly with systemd.services.pict-rs.environment.PICTRS__SERVER__ADDRESS = "127.0.0.1:5934";

@marius851000
Copy link
Contributor

marius851000 commented Aug 21, 2024

Hmm... I have some doubt this migration has not happened well. I find an image I uploaded about a years ago on lemmy, and it return 404. I actually remember it said 0 images were migrated after switch to 0.4.0, but the files files and sled folder were populated (and still are). I’ll go investigate that. (it’s part of the folder I backup, so there is no big risk)

Thanks to @marius851000 for reporting this issue. I assume it probably
changed in 0.5 or something.
@emilazy
Copy link
Member Author

emilazy commented Aug 21, 2024

Perhaps it’s related to the environment variable changes listed in the 0.3 to 0.4 migration guide, in particular PICTRS__OLD_DB__PATH? I could include those in the error message along with the package.

@marius851000
Copy link
Contributor

Indeed, removing the newly created folder sled-repo, setting systemd.services.pict-rs.environment.PICTRS__OLD_DB__PATH = config.services.pict-rs.dataDir; and returning to 0.4, it seems to be migrating now. I’ll leave it running while I sleep.

This was broken by the Rust 1.80 upgrade, and is an old version that
we’d have to patch to keep working.

We have already done the 0.4 → 0.5 update without keeping around
the old version or adding in any additional `stateVersion` logic
in <NixOS#280221>. As a result,
migration for 0.3 users is going to be a little awkward. I’ve done
my best to provide comprehensive instructions for anyone who hasn’t
already bumped to 0.4.

It is probably a footgun to add `stateVersion` logic for any
package that makes backwards‐incompatible schema changes and only
supports migration from the immediately previous version. Users
won’t get migrated by default and we have to either package and
maintain an endlessly growing list of old versions or add complicated
instructions like this. It’s not really practical for us to support
a significantly better migration story than upstream does.
@emilazy emilazy force-pushed the push-yoykzrxwpykp branch from eb03363 to afdee7e Compare August 22, 2024 00:54
@emilazy
Copy link
Member Author

emilazy commented Aug 22, 2024

I’ve updated the PR with the fix and more precise instructions; please let me know how the migration goes and what you think of the new diff :)

Something I am worried about is that now that we’re setting PICTRS__SERVER__ADDRESS, the 0.4 step won’t get PICTRS__SERVER__ADDR set. Should we include that variable in the configuration snippet too so that it’s possible to check that the migration worked before bumping the version again?

Unfortunately, I guess this also means that anyone who opted in to upgrading to 0.4 in the past didn’t actually get things upgraded. There’s probably nothing we can do about that; hopefully they at least noticed before the bump to 0.5…

@marius851000
Copy link
Contributor

I think 0.4 also use PICTRS__SERVER__ADDRESS. At least it doesn’t crash with systemd.services.pict-rs.environment.PICTRS__SERVER__ADDR = pkgs.lib.mkForce null;. It has migrated well (and is properly the image as expected). I have to go to work now, so won’t check more than that. I’ve now returned to 0.5.0, and it’s own migration seems to go well.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

We shouldn't invest much more work here. The hand full of people that use the old version just need to manually migrate.

@emilazy
Copy link
Member Author

emilazy commented Aug 22, 2024

It’s difficult to just manually migrate when Nixpkgs has removed the intermediate version you need to migrate through, but I’m happy with the state of the instructions here. Thanks for all the help, @marius851000!

@emilazy emilazy merged commit 01a749e into NixOS:master Aug 22, 2024
@emilazy emilazy deleted the push-yoykzrxwpykp branch August 26, 2024 00:51
TomaSajt added a commit to TomaSajt/nixpkgs that referenced this pull request Mar 24, 2025
NixOS#336077 only added a throw to
aliases.nix, but didn't remove the files
@TomaSajt TomaSajt mentioned this pull request Mar 24, 2025
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: clean-up This PR removes packages or removes other cruft 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants