yarnpkg / website

Yarn package manager website
https://classic.yarnpkg.com
432 stars 979 forks source link

install.sh: Post-verification cleanup (terminate gpg-agent) #1030

Closed jayaddison closed 4 years ago

jayaddison commented 4 years ago

This changeset terminates any gpg-agent processes created as side-effects during the install.sh script.

If existing gpg-agent processes were found before GPG functionality is invoked in the script, those existing processes will be left untouched so as not to affect unrelated system/user functionality.

Fixes #1029

DanBuild commented 4 years ago

Deploy preview for yarnpkg failed.

Built with commit 4463c83c9dbdc6a470570331d123f9ef6e877fb9

https://app.netlify.com/sites/yarnpkg/deploys/5dee5a3904b544000807d163

jayaddison commented 4 years ago

cc @BanzaiMan - this may help with the issues re: GPG subprocesses you've seen user feedback about (ref: https://github.com/travis-ci/travis-ci/issues/8082#issuecomment-522590424). It's important to make sure this PR doesn't cause side-effects elsewhere though.

jayaddison commented 4 years ago

gpg-connect-agent --no-autostart also looked like a possibility to determine if the agent is current running.

However, trying that with version 2.2.12 of gpg-connect-agent, the agent is still started by a reset process (confirmed via strace), so it may not be reliable.

Daniel15 commented 4 years ago

Seems reasonable to me. Does pgrep come preinstalled on minimal installs of popular Linux distros (Debian, Ubuntu, CentOS)?

jayaddison commented 4 years ago

@Daniel15 Good question, perhaps there's an authority / comparison on which Linux distros it's available under; I'm taking a bit of a look around just now.

jayaddison commented 4 years ago

While searching around elsewhere, here's whether pgrep is available in each of the latest Docker Hub official images for Centos, Debian and Ubuntu:

Alpine

/ # cat /etc/alpine-release 
3.10.3
/ # which pgrep
/usr/bin/pgrep
/ #

CentOS

sh-4.4# cat /etc/centos-release
CentOS Linux release 8.0.1905 (Core) 
sh-4.4# whereis pgrep
pgrep: /usr/bin/pgrep

Debian:

# cat /etc/debian_version
10.2
# which pgrep
#

Ubuntu

# cat /etc/debian_version
buster/sid
# which pgrep
/usr/bin/pgrep
#

So Debian does not seem to have pgrep by default.

Edit - add sources:

jayaddison commented 4 years ago

@Daniel15 Hrm. I'm not finding many resources to compare command-line tooling per-distro; it'd probably take a bit more research. Something like caniuse for command-line tools would be wonderful.. :pray:

One option to handle cases like Debian could be to fall-back to alternatives like the killall command. It doesn't look like yarnpkg currently supports Solaris or other non-Linuxes where that command can be a bit more dangerous, so perhaps that's fine (or even a better alternative than using pkill in the first place).

Whatever approach, I'm keen to be a bit cautious, since this script seems fairly important infrastructure-wise :)

jayaddison commented 4 years ago

@Daniel15 @BanzaiMan Just for transparency; it looks likely that this wasn't the true root cause of Travis CI timeout issues seen in lighthouse during Linux builds.

It still seems useful to clean-up non-install side-effects of the script, so there may still be some value here, but I think the importance & impact of this PR are low.

Glad for any thoughts, and I'll gradually continue on to see if there are more cross-platform alternatives to pgrep and pkill, but there's no urgency (and I don't think it's a magic solution to any other customer issue reports).

jayaddison commented 4 years ago

Investigating pidof and killall as alternatives to pgrep and pkill, respectively:

Alpine

/ # cat /etc/alpine-release 
3.10.3
/ # command -v pidof
/bin/pidof
/ # command -v killall
/usr/bin/killall
/ #

Centos

sh-4.4# cat /etc/centos-release
CentOS Linux release 8.0.1905 (Core) 
sh-4.4# command -v pidof
/usr/sbin/pidof
sh-4.4# command -v killall
sh-4.4#

Debian

# cat /etc/debian_version
10.2
# command -v pidof
/bin/pidof
# command -v killall
# 

Ubuntu

# cat /etc/debian_version
buster/sid
# command -v pidof
/bin/pidof
# command -v killall
# 

Findings:

jayaddison commented 4 years ago

@Daniel15 This turned out not to be the cause of the Travis build failures I was seeing; instead it looks like that was caused by slow cache restoration - so I'll go ahead and close this PR.

Daniel15 commented 4 years ago

Thanks for the update... Feel free to ping me again if you'd like to reopen this at some point.