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

add equivalent dependencies for rhel/centos #547

Closed
wants to merge 3 commits into from
Closed

add equivalent dependencies for rhel/centos #547

wants to merge 3 commits into from

Conversation

joshsleeper
Copy link

@joshsleeper joshsleeper commented Apr 24, 2018

changes

i did some digging and found the packages that add the required libraries on centos 7, so figured i'd suggest adding them to the docs so it's known information.

fixes

fixes #542

notes

technically this doesn't fix any open issues afaik, but I'll mark it as resolving the issue I opened while attempting to test it.

I did some digging and found the packages that add the required libraries on centos 7, so figured I'd suggest adding them to the docs so it's known information.
@CLAassistant
Copy link

CLAassistant commented Apr 24, 2018

CLA assistant check
All committers have signed the CLA.

@kuceb
Copy link
Contributor

kuceb commented Apr 25, 2018

Looks like you also need libXtst, which is also a dependency for google-chrome-stable

```shell
apt-get install xvfb libgtk2.0-0 libnotify-dev libgconf-2-4 libnss3 libxss1 libasound2
```

## RHEL / CentOS
```shell
yum install xorg-x11-server-Xvfb gtk2 libnotify GConf2 nss libXScrnSaver alsa-lib
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add libXtst and a -y so you don't have to manually approve them

Copy link
Author

@joshsleeper joshsleeper Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just matching package for package with the existing apt-get call, which doesn't seem to install libXtst.
Curious, is libXtst required to run the Electron version as well, or only Chrome Stable?

Regardless, if you want me to add libXtst, I'd like to add the equivalent dependency to the apt-get command as well.
Any idea what those packages are, or do I need to hunt em down?

Also, I'm fine adding the -y if you really want it, but I'm of the opinion that this seems to be code that a user would copy and paste to run. As such, I kinda feel like it's more appropriate to let the user manually accept the resulting dependencies. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkucera any thoughts on my comments?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Ubuntu ships with that dependency already installed.
Also since must users come from the npm world, where packages are arguably less vetting and verified, and does not ask to confirm every package individually, I think -y is an okay addition

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, it's required for electron

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jennifer-shehane
any input on this?

  1. should libXtst be added to both lines?
  2. do you agree or disagree with adding -y to the commands?

I'm unsure if the libXtst dep is an actual requirement or not given that all of my systems have it pre-installed and I can't remove it without breaking other dep chains.

I'm also hesitant to add -y to the commands unless the core team likes that approach. @bkucera's stance about most of the users being npm friendly makes sense, but I would counter-point that just because they're npm friendly doesn't mean that the shell should suddenly act like npm. ;)
Either way, I'd defer to your stance on that one.

@@ -121,10 +121,16 @@ See our {% url 'examples' https://on.cypress.io/docker %} for additional informa

If you are not using one of the above CI providers then make sure your system has these dependencies installed.

## Ubuntu / Debian
```shell
apt-get install xvfb libgtk2.0-0 libnotify-dev libgconf-2-4 libnss3 libxss1 libasound2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add -y here too

```shell
apt-get install xvfb libgtk2.0-0 libnotify-dev libgconf-2-4 libnss3 libxss1 libasound2
```

## RHEL / CentOS
```shell
yum install xorg-x11-server-Xvfb gtk2 libnotify GConf2 nss libXScrnSaver alsa-lib
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I was looking at the docs, and I think instead of adding every dependency here in Markdown, without really testing it, we could just

  • keep existing list of Debian dependencies (since this is the most common image I believe)
  • point at our https://github.com/cypress-io/cypress-docker-images repo where the dependencies are actually used to make and test images. In these docs we can say "for the up-to-date list see Docker image "

If a library is needed to run Cypress, one can always open an issue in cypress-docker-images with a test case that fails to execute, and adding the library solves

Copy link
Author

@joshsleeper joshsleeper May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about something like this?

# Dependencies

If you are not using one of the above CI providers then make sure your system has the required dependencies installed.

An up-to-date list of required dependencies for supported platforms can be found in the `Dockerfile` for each platform in the [`cypress-io/cypress-docker-images`](https://github.com/cypress-io/cypress-docker-images) repository

I'm not sure I see the value in keeping only the debian list if we're going to suggest they check the Dockerfiles for an up-to-date list anyway, but if you feel strongly about it I can do that.

edit

upon further thought, I definitely don't like keeping the debian dep list. to your own point, keeping a list of exact deps, untested, here in Markdown, doesn't make a lot of sense. rather than have our debian dep list definitely become out of date at some point, i think we're much better off just directing them to the cypress-docker-images repo so we never have out of date dep docs again.

yay? nay?

@kuceb
Copy link
Contributor

kuceb commented May 1, 2018

It's pretty easy to test the dependencies, you just spin up a vanilla docker container of "your distro here", then install everything. I just used docker run -it centos:7 bash for example.

@joshsleeper
Copy link
Author

Right, but I think @bahmutov's point is that the docs can get out of lockstep with the actual dependencies, whereas referencing the Dockerfiles directly guarantees that when deps change, so do the docs people reference.

@jennifer-shehane
Copy link
Member

Should this be closed @bkucera? Anything to pull from this PR?

@kuceb
Copy link
Contributor

kuceb commented Oct 24, 2018

I think we should be referencing the dockerfiles like @bahmutov mentioned, so I'm for closing this

@joshsleeper
Copy link
Author

Closing as requested!

@joshsleeper joshsleeper deleted the add-centos-dependencies branch October 24, 2018 15:37
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.

npm run test-e2e fails / CONTRIBUTING.md directions incomplete or incorrect
5 participants