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

build: use Ninja in action workflows #37499

Closed
wants to merge 5 commits into from
Closed

Conversation

targos
Copy link
Member

@targos targos commented Feb 24, 2021

Until we have a fix for the makefiles generated by GYP.

Refs: #37429

Until we have a fix for the makefiles generated by GYP.

Refs: nodejs#37429
@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Feb 24, 2021
@targos
Copy link
Member Author

targos commented Feb 24, 2021

Haha I would have thought that Ninja was available by default in GitHub action runners

@targos
Copy link
Member Author

targos commented Feb 24, 2021

actions/runner-images#741 (comment)

@gengjiawen
Copy link
Member

actions/virtual-environments#741 (comment)

haha, same here. In the end, I build this in docker to avoid such issues (https://github.com/gengjiawen/node-build/blob/7bbcd29d624a79cccf3aa7bafd94a5747ae12c48/Dockerfile#L23).
Also suggest add it to gitpods gitpod-io/workspace-images#357.

@gengjiawen
Copy link
Member

Until we have a fix for the makefiles generated by GYP.

If we have fix some makefiles. I still hope we use ninja by default since it's much fast.

@aduh95
Copy link
Contributor

aduh95 commented Feb 24, 2021

It seems it doesn't improve the situation – maybe you need to specify CONFIG_FLAGS=--ninja to the make test command as well?

@targos
Copy link
Member Author

targos commented Feb 24, 2021

It seems it doesn't improve the situation – maybe you need to specify CONFIG_FLAGS=--ninja to the make test command as well?

Looks like you're right. I wonder what's special about github actions here. On my computer, it's enough to just ./configure --ninja and then make test will use Ninja

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Tests completed in less than 10m 🎉

@aduh95
Copy link
Contributor

aduh95 commented Feb 24, 2021

coverage-linux tests are still using GYP and are taking very long to complete.

@targos
Copy link
Member Author

targos commented Feb 24, 2021

@aduh95 right, I missed coverage-linux, thanks!

@richardlau
Copy link
Member

FWIW I think I've found the source of the regression with makefiles. PR: #37502

@aduh95
Copy link
Contributor

aduh95 commented Feb 24, 2021

fast-track?

@aduh95 aduh95 added the fast-track PRs that do not need to wait for 48 hours to land. label Feb 24, 2021
@aduh95
Copy link
Contributor

aduh95 commented Feb 24, 2021

#37505 fixes the issue with GYP – although we probably should still land this and use Ninja on GH Actions considering the improvement for build time.

@richardlau
Copy link
Member

#37505 fixes the issue with GYP – although we probably should still land this and use Ninja on GH Actions considering the improvement for build time.

The build times in #37505 on GitHub Actions are comparable to the times for this PR. I would not expect them to be wildly different as GitHub actions start with a clean workspace/checkout.

@targos targos closed this Feb 24, 2021
@targos targos deleted the actions-ninja branch April 12, 2021 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants