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

Fix libXScrnSaver requirement #5226

Closed
wants to merge 6 commits into from
Closed

Fix libXScrnSaver requirement #5226

wants to merge 6 commits into from

Conversation

marceloatie
Copy link
Contributor

@marceloatie marceloatie commented Dec 30, 2016

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

Make dependency independent of package name
@cjb
Copy link
Contributor

cjb commented Apr 19, 2017

Thanks for the PR! I'm sorry we missed this. CC @oconnor663 to help review, fixes the RPM on OpenSUSE.

@oconnor663 oconnor663 self-assigned this Apr 19, 2017
@oconnor663
Copy link
Contributor

When I try this on my 64-bit Fedora VM, it looks like it tries to install a bunch of 32-bit packages:

[jacko@localhost ~]$ sudo dnf install libXss.so.1
keybase                                                                             41 kB/s | 5.1 kB     00:00    
Dependencies resolved.
===================================================================================================================
 Package                          Arch                 Version                         Repository             Size
===================================================================================================================
Installing:
 glibc                            i686                 2.24-4.fc25                     updates               4.0 M
 libX11                           i686                 1.6.4-4.fc25                    updates               623 k
 libXScrnSaver                    i686                 1.2.2-10.fc24                   fedora                 28 k
 libXScrnSaver                    x86_64               1.2.2-10.fc24                   fedora                 28 k
 libXau                           i686                 1.0.8-6.fc24                    fedora                 34 k
 libXext                          i686                 1.3.3-4.fc24                    fedora                 43 k
 libcrypt-nss                     i686                 2.24-4.fc25                     updates                43 k
 libxcb                           i686                 1.12-1.fc25                     fedora                227 k
 nss-softokn-freebl               i686                 3.29.3-1.0.fc25                 updates               221 k

Transaction Summary
===================================================================================================================
Install  9 Packages

Total download size: 5.3 M
Installed size: 17 M
Is this ok [y/N]:

It looks like some obscure parentheses syntax can fix this?

[jacko@localhost ~]$ sudo dnf install 'libXss.so.1()(64bit)'
Last metadata expiration check: 0:00:19 ago on Wed Apr 19 11:33:24 2017.
Dependencies resolved.
===================================================================================================================
 Package                       Arch                   Version                         Repository              Size
===================================================================================================================
Installing:
 libXScrnSaver                 x86_64                 1.2.2-10.fc24                   fedora                  28 k

Transaction Summary
===================================================================================================================
Install  1 Package

Total download size: 28 k
Installed size: 39 k
Is this ok [y/N]:

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?

@marceloatie
Copy link
Contributor Author

openSUSE 42.2

sudo zypper install libXss.so.1
Loading repository data...
Reading installed packages...                                            
'libXss.so.1' not found in package names. Trying capabilities.           
Resolving package dependencies...

The following 5 NEW packages are going to be installed:
  libX11-6-32bit libXau6-32bit libXext6-32bit libXss1-32bit libxcb1-32bit

"libXss1' providing 'libXss.so.1()(64bit)' is already installed"

sudo zypper install 'libXss.so.1()(64bit)'
Loading repository data...
Reading installed packages...
'libXss.so.1()(64bit)' not found in package names. Trying capabilities.
'libXss1' providing 'libXss.so.1()(64bit)' is already installed.
Resolving package dependencies...

Nothing to do.

@oconnor663 I think it will be portable. Thanks by report the installation of 32-bit packages.

@michbona
Copy link

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.
This is the description of the package in question and I seriously don't get why keybase would depend on that:

libXss1 - X11 Screen Saver extension client library
The X Window System provides support for changing the image on a display screen after a user-settable period of inactivity to avoid burning the cathode ray tube phosphors. This extension allows an external "screen saver" client to detect when the alternate image is to be displayed and to provide the graphics.

@oconnor663
Copy link
Contributor

@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?

@michbona
Copy link

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.

@cjb
Copy link
Contributor

cjb commented Apr 21, 2017

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.

@Rewarp
Copy link

Rewarp commented Jun 13, 2017

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?

@oconnor663
Copy link
Contributor

@cjb any chance Electron started statically linking it?

@cjb
Copy link
Contributor

cjb commented Jun 13, 2017

@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.

@michbona
Copy link

michbona commented Sep 27, 2017

This bug still exists in keybase version 1.0.32-20170926160055+865e3f695
Sorry, this discussion seems be happening in Issue keybase/keybase-issues#2445 as well, but since the proposed patch is here, I will add my 2 cents here. @cjb : I appreciate that new features like Teams are great and take a lot of attention, but can we please solve this pesky little bug for some early supporters? It's really hard to believe that a security-focused app like keybase cannot provide auto-updates on a major Linux distro. And I am not talking about the some weird Zypper bug like keybase/keybase-issues#2569, this one is on your package.

@cjb
Copy link
Contributor

cjb commented Sep 27, 2017

@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.

@oconnor663
Copy link
Contributor

It's really hard to believe that a security-focused app like keybase cannot provide auto-updates on a major Linux distro.

The very latest change to this PR (just now, as I'm writing this comment) probably does the right thing, with an if statement that specifies different dependencies for different architectures.

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.

@oconnor663
Copy link
Contributor

@marceloatie thanks a million for the work you're doing on this.

@michbona
Copy link

michbona commented Oct 2, 2017

@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.

@marceloatie
Copy link
Contributor Author

This week i will build updated Kaybase on Jenkins to fix this error, please wait just a bit.

@oconnor663
Copy link
Contributor

oconnor663 commented Oct 6, 2017

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)

oconnor663 pushed a commit that referenced this pull request Dec 19, 2017
Fix by @marceloatie, originally in #5226,
squashed and merged by @oconnor663.
oconnor663 pushed a commit that referenced this pull request Dec 21, 2017
Fix by @marceloatie, originally in #5226,
squashed and merged by @oconnor663.
@oconnor663
Copy link
Contributor

Just merged these changes as part of #9996. The next public release probably won't be until after the holidays though.

@oconnor663 oconnor663 closed this Dec 21, 2017
@oconnor663
Copy link
Contributor

We just pushed a release with this fix. Please let me know if you get a chance to try it.

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

Successfully merging this pull request may close these issues.

5 participants