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

chore: Update installation requirements in CONTRIBUTING.md #27878

Merged
merged 3 commits into from
Sep 25, 2023

Conversation

Crustum7
Copy link
Contributor

Additional details

CMake is needed to set up the developer environment

Steps to test

na

How has the user experience changed?

na

PR Tasks

@cypress-app-bot
Copy link
Collaborator

@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Sep 22, 2023

@Crustum7

  • Thanks for picking this up!
  • I think you need to change the commit title in 73d4058 which says "Solves issue-27846" and this is not a semantic commit message. I would suggest: docs: update installation requirements in contributing.md instead.

Copy link
Contributor

@alexsch01 alexsch01 left a comment

Choose a reason for hiding this comment

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

instead of on Unix, it should be on Debian-based systems

@alexsch01
Copy link
Contributor

@Crustum7 installing just cmake didn't fix the build issue

@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Sep 22, 2023

@alexsch01

installing just cmake didn't fix the build issue

Is there something missing if you followed the prerequisites in https://github.com/nodejs/node-gyp#on-unix?

@Crustum7
Copy link
Contributor Author

@alexsch01 well as @MikeMcC399 mentioned here, make and build-essential are indirectly required through node-gyp so they should not need to be included here as well. I can change to "on Debian-based systems" though if you want

@alexsch01
Copy link
Contributor

I made a PR that supersedes this one: #27879
g++ and make were not required to build Cypress before v13.0.0

@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Sep 22, 2023

If cmake is not installed then yarn install fails at:

  1. rebuild of cpu-features@0.0.2
  2. electron-rebuild -o better-sqlite3

so I support adding it explicitly to the list of requirements to build Cypress. My understanding of reading https://github.com/nodejs/node-gyp#on-unix is that it is not specifically mentioned there. If I have misinterpreted that, then I'd be grateful for an explanation.


https://github.com/nodejs/node-gyp#on-unix

On Unix

Copy link
Contributor

@MikeMcC399 MikeMcC399 left a comment

Choose a reason for hiding this comment

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

LGTM

@MikeMcC399
Copy link
Contributor

@alexsch01

I made a PR that supersedes this one: #27879 g++ and make were not required to build Cypress before v13.0.0

I believe that is unnecessary, since CONTRIBUTING > Requirements refers to

python (since we use node-gyp. See their repo for Python version requirements.) which refers to

These generic instructions are admittedly difficult to follow. Perhaps there should be a cook-book with some explicit instructions for operating systems commonly used to build Cypress, like for instance Ubuntu 22.04 LTS?

I would in any case support merging this PR to simply add cmake to the list of requirements and then separately work on the issue about making the instructions easier to understand and use.

@alexsch01
Copy link
Contributor

alexsch01 commented Sep 22, 2023

cmake isn't required for the yarn command to successfuly install Cypress

g++ and make are required on Linux


A proper C/C++ compiler toolchain, like GCC

GCC is not enough for Cypress to build successfully


build-essential package on debian solved my building issues because it includes g++ and make

@MikeMcC399
Copy link
Contributor

@alexsch01

Thanks for your research and explanation. I will check again on a clean install of Ubuntu if build-essential is enough. I was getting errors without cmake, however I may have overlooked something. According to https://packages.ubuntu.com/jammy/build-essential it contains g++, gcc and make so perhaps this would be a good package to explicitly recommend for Debian-based systems?

@alexsch01
Copy link
Contributor

alexsch01 commented Sep 22, 2023

@alexsch01

Thanks for your research and explanation. I will check again on a clean install of Ubuntu if build-essential is enough. I was getting errors without cmake, however I may have overlooked something. According to https://packages.ubuntu.com/jammy/build-essential it contains g++, gcc and make so perhaps this would be a good package to explicitly recommend for Debian-based systems?

My PR has g++ (this includes gcc) and make required

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MikeMcC399 MikeMcC399 left a comment

Choose a reason for hiding this comment

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

The requirements for running node-gyp are supposed to be covered in the requirements section referring to python. In practice the instructions are too vague and too open to interpretation, apart from the explicit mention of make, so I suggest to make the text look like this, with an note especially for Debian-based systems (including Ubuntu):

  • python (since we use node-gyp. See their repo for Python version requirements.)
    • Note for Debian-based systems: python is pre-installed.
      sudo apt install g++ make cmake meets the additional requirements to run node-gyp in the context of building Cypress from source.

Co-authored-by: Mike McCready <66998419+MikeMcC399@users.noreply.github.com>
Copy link
Contributor

@MikeMcC399 MikeMcC399 left a comment

Choose a reason for hiding this comment

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

Thank you very much @Crustum7! 👍🏻

I'm confident that this is correct advice now and should help anybody trying to set up a new Debian-based system to build Cypress from source.

It would certainly have helped me when I first did it last year and was struggling with trial-and-error methods!

@MikeMcC399
Copy link
Contributor

@Crustum7

Just for future reference if you create any new PR, there are some instructions in https://github.com/cypress-io/cypress/blob/develop/CONTRIBUTING.md#pull-requests about naming the branch. It's not good practice to make edits in the default develop branch, even if you are working from a fork. Unfortunately you can't change the source branch name of an open PR, you can only change the target branch name, so don't worry about this for the moment. Just leave it as it is!

@@ -207,6 +207,7 @@ You must have the following installed on your system to contribute locally:
- [`Node.js`](https://nodejs.org/en/) (See the root [.node-version](.node-version) file for the required version. You can find a list of tools on [node-version-usage](https://github.com/shadowspawn/node-version-usage) to switch the version of [`Node.js`](https://nodejs.org/en/) based on [.node-version](.node-version).)
- [`yarn`](https://yarnpkg.com/en/docs/install)
- [`python`](https://www.python.org/downloads/) (since we use `node-gyp`. See their [repo](https://github.com/nodejs/node-gyp) for Python version requirements.)
- Note for Debian-based systems: `python` is pre-installed.<br>`sudo apt install g++ make cmake` meets the additional requirements to run `node-gyp` in the context of building Cypress from source.

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually nope, make should still be in the install command

Sorry for the confusion

Copy link
Contributor

Choose a reason for hiding this comment

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

I've hidden the latest suggestion. I'm not an apt expert. I have just been checking that the instructions work on Ubuntu.

@nagash77 nagash77 merged commit dbfb3e6 into cypress-io:develop Sep 25, 2023
2 of 5 checks passed
@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Sep 25, 2023

Just as a sanity cross-check, the following instructions will build Cypress on a freshly-installed Ubuntu 22.04 system with no other prerequisites installed:

sudo apt install git g++ cmake make curl
curl -L https://bit.ly/n-install | bash
 . ~/.bashrc
cd ~
git clone https://github.com/cypress-io/cypress
cd cypress
n auto
npm install yarn -g
yarn

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.

Can't build v13.0.0+ from source without g++ and make
6 participants