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

kubo-migrator: add migration from 12 to 13 #209304

Merged
merged 2 commits into from
Feb 27, 2023

Conversation

Luflosi
Copy link
Contributor

@Luflosi Luflosi commented Jan 6, 2023

Description of changes

https://github.com/ipfs/fs-repo-migrations/releases/tag/fs-repo-12-to-13%2Fv1.0.0
This will be used to upgrade the Kubo repo when updating to Kubo 0.18.

Two old migrations were broken since Go 1.16 was removed from Nixpkgs.
Upstream is not interested in fixing the migrations to work with newer Go versions: ipfs/fs-repo-migrations#163 (comment).

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from elitak January 6, 2023 13:18
@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 Jan 6, 2023
@milahu
Copy link
Contributor

milahu commented Jan 11, 2023

Replace them with a stub as discussed in #168480 (comment), so that fs-repo-migrations still finds the binaries in the PATH and never attempts to download these binaries from the internet, because that is a bit uncool.

this fails because ld-linux.so is missing

If anyone still wants to actually use these migrations, they should use an old version of Nixpkgs that still includes Go 1.16.

not practical, as this will compile go_1_16 = slow

the fix is as simple as ipfs/fs-repo-migrations#163

@Luflosi Luflosi marked this pull request as draft January 11, 2023 23:31
@Luflosi
Copy link
Contributor Author

Luflosi commented Jan 12, 2023

this fails because ld-linux.so is missing

From reading what you linked to, it sounds like you ran ipfs daemon with a repo, which required a migration. The migration binary fs-repo-11-to-12 was not in the $PATH, so Kubo downloaded it off the internet, which then failed to run. It doesn't surprise me at all that random binaries off of the internet don't work on NixOS because NixOS does not implement the FHS. As such I don't think the downloaded binary not working on NixOS is an issue in Kubo. Please make that clear in the linked issue.
Kubo would not have tried to download a binary if you had put kubo-migrator-all-fs-repo-migrations into its $PATH. I replaced all the old migrations instead of deleting them to try to prevent such a situation.
Anyways, my comment only mentions fs-repo-migrations, which you did not run and so I don't understand what your comment is about. Did I understand something incorrectly?

not practical, as this will compile go_1_16 = slow

This is not true, everything is still in the cache. To test for yourself:

git checkout 4c3c80df545ec5cb26b5480979c3e3f93518cbe5
nix-build -A ipfs-migrator

I got this commit hash from the list at https://channels.nix.gsc.io/nixpkgs-unstable/history. The timestamp is from a day before Go 1.16 was removed.

the fix is as simple as ipfs/fs-repo-migrations#163

Thanks, I will try to incorporate this patch into this PR when I have some time.
You only fixed the 10-to-11 and 11-to-12 migrations. What about the older migrations? Can they not be fixed in the same way or would it be more effort?

@milahu
Copy link
Contributor

milahu commented Jan 12, 2023

What about the older migrations?

see my PR

From reading what you linked to, it sounds like you ran ipfs daemon with a repo, which required a migration.

yes

The migration binary fs-repo-11-to-12 was not in the $PATH, so Kubo downloaded it off the internet, which then failed to run.

i run

nix-shell -p ipfs kubo-migrator # aka: ipfs-migrator
which fs-repo-migrations
# /nix/store/af47hh83i6awalxhrfdpr40gks5cm0fi-kubo-migrator-2.0.2/bin/fs-repo-migrations
ipfs daemon

so ipfs should use fs-repo-migrations

it works with

nix-shell -p ipfs kubo-migrator-all-fs-repo-migrations
which fs-repo-11-to-12
# /nix/store/60fnfclibg8mn0f0s55yv6v07dz7vh4c-kubo-migrator-all-fs-repo-migrations-12.1.0.2/bin/fs-repo-11-to-12
ipfs daemon

@Luflosi
Copy link
Contributor Author

Luflosi commented Jan 24, 2023

Ok, it should work now, thanks again for the patch.
I had to create a separate derivation for the source. This is because I'm using using sourceRoot inside the derivation which builds the individual migrations and that would otherwise require me to change the patches to apply to the child directories instead and to apply each patch only to the corresponding migration. Using a separate derivation just for patching is easier.

@Luflosi Luflosi marked this pull request as ready for review January 24, 2023 20:12
@Luflosi Luflosi mentioned this pull request Jan 24, 2023
13 tasks
Two old migrations were broken since Go 1.16 was removed from Nixpkgs.
Upstream is not interested in fixing the migrations to work with newer Go versions: ipfs/fs-repo-migrations#163 (comment).
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/850

@SuperSandro2000 SuperSandro2000 merged commit 7dce27f into NixOS:master Feb 27, 2023
@Luflosi Luflosi deleted the update/kubo-migrator branch February 27, 2023 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants