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

seadrive-gui: init at 1.0.7, seadrive-daemon: init at 1.0.7 #66540

Closed
wants to merge 30 commits into from

Conversation

@JustinLovinger
Copy link
Contributor

JustinLovinger commented Aug 12, 2019

Motivation for this change

SeaDrive is the new client for the Seafile cloud syncing service. Unlike the original seafile-client, SeaDrive can download files on-demand, allowing access to your entire Seafile library on devices with low storage space.

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 nix-review --run "nix-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

JustinLovinger added 14 commits Aug 5, 2019
And various Debian libcurl requirements with Debian versions
To fix library version mismatch errors
To fix errors with glibc
Debian buster version is built with a newer version of glibc than NixOS
has
Nested directories were more difficult to follow and odd when a library
is used by multiple packages
In installPhase of seadrive-daemon libs
No need for duplicate files
Instead of pulling from github
To avoid an additional network operation and to avoid relying on a very
specific git commit
To fit contribution guidelines
seadrive-daemon: add maintainer
Copy link
Contributor

alexarice left a comment

I don't understand a lot of this and am not sure why you need all the debian libraries but have tried to put some useful comments in

@aanderse
Copy link
Contributor

aanderse commented Aug 13, 2019

The website says "open source" and has a github link. Can we not grab the source and compile?

@JustinLovinger
Copy link
Contributor Author

JustinLovinger commented Aug 13, 2019

@aanderse seadrive-daemon is not currently open source. seadrive-gui, seafile-client, and the Seafile server are open source.

Copy link
Contributor

aanderse left a comment

Thanks for choosing to contribute to nixpkgs! There are a few things to work through on this PR before we can merge but there are plenty of people who can answer questions you might have.

The initial questions/concerns with this PR for me are all of the custom package versions. Sometimes packages (especially proprietary ones) have fairly specific requirements and custom libs are a part of that, but often times we can work around those limitations using the tools we have at our disposal in nixpkgs.

Please see the questions/comments I have left as a review on this PR and let me know if you require any clarification.

nixpkgs cmake adds the "-DCMAKE_BUILD_TYPE=Release" flag by default.
A clearner solution to allow seadrive-gui to call seadrive-daemon at runtime.
@JustinLovinger
Copy link
Contributor Author

JustinLovinger commented Aug 19, 2019

@aanderse Do you have any more recommendations? As for the bundled Debian libraries, this was the only way I could find to fix the library mismatch errors. I added the smallest set of bundled libraries to eliminate all errors and warnings.

@aanderse
Copy link
Contributor

aanderse commented Aug 20, 2019

@JustinLovinger I'm not very good with patchelf but wouldn't something like this possibly work?

@JustinLovinger
Copy link
Contributor Author

JustinLovinger commented Aug 20, 2019

@aanderse It looks like you are trying to trick the program into thinking it has a different version of the library. This was actually discussed as a possible solution on the #nixpkgs irc, but was deemed risky, especially since we are dealing with a data syncing service and don't want to risk losing data.

I just tried running it. It doesn't look like it fixes the warnings

./result/bin/seadrive: /nix/store/m93jjbdb9nvc1f38fxbf14rs4r1g013a-curl-7.65.3/lib/libcurl.so.4: no version information available (required by ./result/bin/seadrive)
./result/bin/seadrive: /nix/store/inp0f3gmcq8jxmxqa3spdp9g3zp6rvda-seadrive-daemon-1.0.6/lib/libcrypto.so.1.0.2: no version information available (required by ./result/bin/seadrive)

Although the SeaDrive logs don't show any obvious errors, the data itself does not appear to be syncing. No files or Seafile libraries are available.

Also, are you sure replacing the bundled libevent_2_0 with an override like that is a good idea? It seems fragile. The latest libevent could make a change that breaks this libevent_2_0, or worse, breaks seadrive-daemon without breaking libevent_2_0, leading to a difficult to track down problem. With the bundled libevent_2_0 nix, at least we know it will always be the same package, unless we specifically change it.

@aanderse
Copy link
Contributor

aanderse commented Aug 20, 2019

@JustinLovinger fair enough. I thought I'd at least give it a try.

Who were you working with on IRC? I'd like them to give this a review.

@JustinLovinger
Copy link
Contributor Author

JustinLovinger commented Aug 20, 2019

@aanderse The main helpers on irc were srhb, clever, and symphorien. Those were their names on irc.

@aanderse
Copy link
Contributor

aanderse commented Aug 20, 2019

Ah, great. If @srhb is telling you what to do then it is definitely the right answer. I'll leave the remainder of the review to them.

@srhb
Copy link
Contributor

srhb commented Aug 21, 2019

Hi @JustinLovinger! Phew, you sure had to jump through a lot of hoops to make the daemon work. 😁

I'm sorry to say that I feel that it's a bit too many hoops to be currently included in nixpkgs proper, especially considering the potentially high risk libraries like sasl and openssl. I suggest splitting out the open source / built-from-source bits into their own PR and letting the daemon and things live in your own repo such that people may opt into it.

and remove binary library dependencies.
The CentOS binary of seadrive-daemon has better compatibility with nix libraries.
@JustinLovinger JustinLovinger changed the title seadrive-gui: init at 1.0.7, seadrive-daemon: init at 1.0.6 seadrive-gui: init at 1.0.7, seadrive-daemon: init at 1.0.7 Aug 23, 2019
@JustinLovinger
Copy link
Contributor Author

JustinLovinger commented Aug 23, 2019

@srhb Given your concerns about these Debian libraries, I took another look at removing them. I tried replacing the Debian binary of seadrive-daemon with the CentOS binary. Looks like the CentOS binary has better compatability with nix libraries :D. seadrive-daemon can now run using only nix libraries as dependencies.

There is still a warning when seadrive-daemon runs

/nix/store/7dsisl4gxr16rfa53k8c2grayzdmqz0w-seadrive-daemon-1.0.7/lib/libcrypto.so.10: no version information available (required by /tmp/result/bin/seadrive)

but it doesn't seem to result in any runtime errors. I believe the warning is benign, and caused by a lack of, or difference in, an identifier string in libcrypto.so.1.0.0

Also, a happy side effect of switching to the CentOS binary is that seadrive-daemon is bumped to version 1.0.7. Apparently CentOS has a newer version than Debian.

@srhb
Copy link
Contributor

srhb commented Aug 24, 2019

This definitely looks better. However, I'm not sold on the premise that the warning is completely safe. Can you provide some more evidence of this?

Furthermore I'm not sure I like making the old libevent version local to this drv. We might want to keep it in top-level, alongside the newer one. It is still maintained as far as I know.

I'll try and invite some more people to weigh in as well, I don't feel completely comfortable making these calls on my own. :)

@JustinLovinger
Copy link
Contributor Author

JustinLovinger commented Aug 24, 2019

@srhb I think I tracked down what is happening with the "no version information available" warning. To begin, this warning is somewhat deceptively named. The warning actually means that an internal version identifier, provided through an SSLeay() function, does not match what is expected. This warning is known to occur when you run a binary with an OpenSSL library that it was not built with, because the version identifier includes information such as the build platform and even when the library was built. All the warning really tells us is that we are not using the exact copy of OpenSSL that the binary was built with, which is what we want.

Even if the version does match exactly and the built on date and platform is not an issue, we may still get this warning because RHEL (and consequently, CentOS) change the behavior of SSLeay() in their OpenSSL libraries, because they believe that "Some applications perform incorrect (too sensitive) version check and the only way to avoid it is to mask the actual run-time version of OpenSSL built against an old OpenSSL release.".

Also, I have personally been running this build of SeaDrive without error, and with heavy usage, for the past few days :)

As for libevent_2_0, making it a top level package was my first thought as well, so I have no problem with that if it is what people think is best.

@srhb
Copy link
Contributor

srhb commented Aug 24, 2019

@JustinLovinger Thanks for tracking that down. That sets my mind at ease a bit. :)

@JustinLovinger
Copy link
Contributor Author

JustinLovinger commented May 21, 2020

I'm closing this pull request because I'm not using SeaDrive anymore. If anyone wants to pick this up or use SeaDrive themselves, you can find the latest changes here: https://github.com/JustinLovinger/nixpkgs/tree/init-seadrive. I give permission for anyone to make a new pull request with these changes, with maintainer changed, and proper attribution of course :) It worked when I last tested it, but that was several months ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.