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

keybase: on Linux, symlink kbfsfuse, git-remote-keybase into main package #55679

Closed
wants to merge 1 commit into from

Conversation

thoughtpolice
Copy link
Member

@thoughtpolice thoughtpolice commented Feb 13, 2019

Motivation for this change

This fixes some innocuous errors when keybase tries to detect the version kbfs, which it does by default now to warn of upgrade compatibility. Furthermore including git-remote-keybase seems rather sensible, considering I (at least) expected the 'keybase' application to simply include these things

This does have the downside of bloating the closure of keybase by about ~100mb on x86_64 Linux.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

…kage

This fixes some innocuous errors when keybase tries to detect the
version kbfs, which it does by default now to warn of upgrade
compatibility.  Furthermore including git-remote-keybase seems rather
sensible, considering I (at least) expected the 'keybase' application to
simply include these things

Signed-off-by: Austin Seipp <aseipp@pobox.com>
@thoughtpolice
Copy link
Member Author

@GrahamcOfBorg build keybase

@worldofpeace
Copy link
Contributor

@thoughtpolice I don't think we should be doing this, see #50999 (comment).

@thoughtpolice
Copy link
Member Author

@worldofpeace I see, so we shouldn't even need this patch, since it should just be done for us -- and the build of keybase will contain everything we need? Is that my understanding? (FWIW, the kbfs version check here is actually non-fatal, but nice to fix, so it can be left for now)

If that's the case then I agree we shouldn't do this -- we should instead update keybase and drop the kbfs package instead, and ensure keybase includes working git-remote-keybase, etc. (In particular I didn't see any direct mention as to whether they will ship binaries with the same name or e.g. if they might fold kbfsfuse into a keybase fuse command or anything. So we'll at least need to look at the changes more closely to be sure.

@worldofpeace
Copy link
Contributor

@worldofpeace I see, so we shouldn't even need this patch, since it should just be done for us -- and the build of keybase will contain everything we need? Is that my understanding? (FWIW, the kbfs version check here is actually non-fatal, but nice to fix, so it can be left for now)

Yes this is true, they have everything there, we might even eventually be able to build the gui from source.

If that's the case then I agree we shouldn't do this -- we should instead update keybase and drop the kbfs package instead, and ensure keybase includes working git-remote-keybase, etc. (In particular I didn't see any direct mention as to whether they will ship binaries with the same name or e.g. if they might fold kbfsfuse into a keybase fuse command or anything. So we'll at least need to look at the changes more closely to be sure.

Looks like arch linux splits the package, I tend to like that approach.

@worldofpeace worldofpeace mentioned this pull request Mar 1, 2019
10 tasks
@jonringer
Copy link
Contributor

@worldofpeace @thoughtpolice is this still relevant? Seems like keybase packages have changed a lot. See #64248 #64377

@worldofpeace
Copy link
Contributor

I'm not sure if we want everything in one main package.

@worldofpeace
Copy link
Contributor

This is no longer relevant.

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.

None yet

4 participants