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

CMake: support ninja as a build backend #626

Merged
merged 2 commits into from
Jun 10, 2022

Conversation

PragmaTwice
Copy link
Member

Close #621.

Benchmark between make and ninja build for kvrocks can be found in #625.
TL;DR: The benchmark shows that ninja is slightly slower than make build in clean build, but has huge advantages in incremental build, which is very friendly to kvrocks developers who need to do frequent incremental builds.

So in this PR, kvrocks still use make by default, but the problem that blocks ninja backend to successfully build in cmake is fixed and now build.sh has a --ninja option to enable ninja.

git-hulk
git-hulk previously approved these changes Jun 9, 2022
Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

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

LGTM, nice.

@git-hulk git-hulk requested review from ShooterIT and tisonkun June 10, 2022 01:59
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Shall we add a matrix to test build with ninja in CI workflow?

@PragmaTwice
Copy link
Member Author

PragmaTwice commented Jun 10, 2022

Shall we add a matrix to test build with ninja in CI workflow?

I think we could add a ninja job to the daily ci now, but since the workflows haven't been refactored, it might not be easy to use an unified matrix, which is described in #572, and I will make a PR to refactor workflows later.

@tisonkun tisonkun merged commit badd9d7 into apache:unstable Jun 10, 2022
@tisonkun
Copy link
Member

Thanks for your contribution!

@ShooterIT
Copy link
Member

Hi @PragmaTwice Currently daily CI is failed, please have a look,
one tips for debugging daily CI on github, you can remove the rule of daily https://github.com/apache/incubator-kvrocks/blob/unstable/.github/workflows/daily-ci.yaml#L21 in you branch, you can submit a PR after there is no error report.

@tisonkun
Copy link
Member

Actually I think whether we should running a daily CI or just run it every PR - with Apache INFRA we may have enough resource to run CI now. At least we may try to trigger daily CI when related files touched (however, "related files" can be any source files and thus it becomes a CI runs for every PR).

@ShooterIT
Copy link
Member

Hi @tisonkun it will cost much time if we run also daily CI for every PR, i think it is not friendly for contributors.

At least we may try to trigger daily CI when related files touched (however, "related files" can be any source files and thus it becomes a CI runs for every PR).

This idea sounds good, could we support this feature?

@tisonkun
Copy link
Member

At least we can add:

name: Daily CI
on:
  pull_request:
    paths:
      - ".github/workflows/daily-ci.yaml"
  schedule:
    - cron: '0 1 * * *'
  workflow_dispatch:

If I find can find some spare time I'll make some investigation. But feel free to do it yourself.

@PragmaTwice
Copy link
Member Author

PragmaTwice commented Jun 14, 2022

Actually I think whether we should running a daily CI or just run it every PR - with Apache INFRA we may have enough resource to run CI now. At least we may try to trigger daily CI when related files touched (however, "related files" can be any source files and thus it becomes a CI runs for every PR).

I think we could use ccache and actions/cache to accelerate the build procedure in CI, so that more jobs could be added to the workflows triggered by PRs.

Hi @PragmaTwice Currently daily CI is failed, please have a look, one tips for debugging daily CI on github, you can remove the rule of daily https://github.com/apache/incubator-kvrocks/blob/unstable/.github/workflows/daily-ci.yaml#L21 in you branch, you can submit a PR after there is no error report.

Sorry for that, seems there is no pre-installed ninja in the virtual environment, some apt install is needed.

@ShooterIT
Copy link
Member

name: Daily CI
on:
  pull_request:
    paths:
      - ".github/workflows/daily-ci.yaml"
  schedule:
    - cron: '0 1 * * *'
  workflow_dispatch:

thanks @tisonkun cool, i am not familiar CI, but it seems it is a light and useful way to resolve our problem.

@PragmaTwice please don't say sorry, you already have done much for Kvrocks. Just let's fix it, we can adopt @tisonkun advise as above to check.

PS: In redis repo, maintainers can trigger manually daily CI, maybe it also is a way for us.

@PragmaTwice
Copy link
Member Author

PragmaTwice commented Jun 14, 2022

In redis repo, maintainers can trigger manually daily CI, maybe it also is a way for us.

We can also trigger the daily CI manually since workflow_dispatch is added to https://github.com/apache/incubator-kvrocks/blob/unstable/.github/workflows/daily-ci.yaml#L24.

Just like this, click the Run workflow button from a specific workflow page:
image

@ShooterIT
Copy link
Member

found, cool, thanks @PragmaTwice

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.

Enhancement: support Ninja as a CMake backend
4 participants