-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix libXScrnSaver requirement #5226
Conversation
Make dependency independent of package name
Thanks for the PR! I'm sorry we missed this. CC @oconnor663 to help review, fixes the RPM on OpenSUSE. |
When I try this on my 64-bit Fedora VM, it looks like it tries to install a bunch of 32-bit packages:
It looks like some obscure parentheses syntax can fix this?
But I'm not able to find any actual documentation on how the parentheses work. Do you know if this would be the right thing to do on 64-bit systems? Will it be portable across other RPM-based distros? |
openSUSE 42.2
"libXss1' providing 'libXss.so.1()(64bit)' is already installed"
@oconnor663 I think it will be portable. Thanks by report the installation of 32-bit packages. |
Sorry for the stupid question - but is the dependency required at all? I know that tons of packages do have that requirement but I never understood why.
|
@michbona Electron won't start without linking against this library. It's possible there might be workarounds for this problem that I don't know about yet though? |
OK, if it's an Electron thing, I understand the link to a library that protects cathode ray tubes ;-) Thanks for the info. I have no experience with Electron, so I can't help with a workaround. Making the dependency distri independent seems to be the way forward. |
We get it from Electron, Electron gets it from Chromium, and Chromium uses it as one of many inputs for determining whether the system is currently idle. |
I ignored the dependencies on my openSUSE Tumbleweed installaion for libXScrnSaver and keybase launches dandily. Is this really required now? Or am I missing features? |
@cjb any chance Electron started statically linking it? |
@oconnor663 Possible, but even if they had, we don't take on new Electron versions quickly, so I don't expect that's it. Perhaps openSUSE has this dep but provides it in a way that doesn't satisfy our ask? Dunno. |
This bug still exists in keybase version 1.0.32-20170926160055+865e3f695 |
@michbona Sorry, we're a small team, someone who actually uses openSUSE will need to help fix it. The proposed patch here doesn't work -- it installs 32-bit libs. And when we switched to it to asking for 64-bit libs, it seemed like it didn't install anything. |
changed from 32bits to 64bits libraries
The very latest change to this PR (just now, as I'm writing this comment) probably does the right thing, with an Testing is the real issue here, and that's why we didn't just land a fix like this when the problem was first reported. The variance between RPM implementations in particular is really wide -- take a look at the yum/yast/urpmi switches all over this file (mostly copied from Chrome's packaging). Various versions of various distros tend to have maddening little differences that have burned us before. Hardcoding a library filename feels like precisely the kind of thing that will break someone somewhere someday. If anyone with RPM packaging experience could suggest a less-dangerous-sounding workaround, that would be a huge help for us getting this in quickly. |
@marceloatie thanks a million for the work you're doing on this. |
@cjb @oconnor663 I understand the different priorities and the complexity of the problem. And I appreciate every minute you spend solving the problem or at least communicating. But @marceloatie has offered to help, submitted a patch and then had to almost beg for it to be considered. And while I am happy to test stuff, it's really hard to test something that is not in the package. Looking forward to some quicker turnaround times in the futures. After all, I am sure you're not the first to have that particular problem - especially if it's really caused by Electron. |
This week i will build updated Kaybase on Jenkins to fix this error, please wait just a bit. |
I've put this ticket at the top of our sprint for next week. Apologies for all the delay. Edit Nov 27: It ended up getting bumped from that sprint. I've put it back in the next one. Edit Dec 12: (note to self, internal ticket) |
Fix by @marceloatie, originally in #5226, squashed and merged by @oconnor663.
Fix by @marceloatie, originally in #5226, squashed and merged by @oconnor663.
Just merged these changes as part of #9996. The next public release probably won't be until after the holidays though. |
We just pushed a release with this fix. Please let me know if you get a chance to try it. |
openSUSE and some others distributions uses a different name for package libXScrnSaver, it's called libXss1.
This fix uses the name of the module provided by the package, this will make dependency independent of package name.
keybase/keybase-issues#2445