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

Update Contributing docs #4424

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ShohamBit
Copy link
Collaborator

1. Explain what the PR does

This PR is about #4406

  • remove and add the documentation to the guidelines
  • add sections on how to contribute to tracee source code
  • improve docs on how to build tracee
  • improve docs on how to build environment

"Replace me with make check-pr output"

2. Explain how to test it

Read the docs and check if its properly align with tracee design

3. Other comments

Close #4406

Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

First pass.

docs/contributing/building/building.md Show resolved Hide resolved
docs/contributing/building/building.md Show resolved Hide resolved
docs/contributing/building/environment.md Outdated Show resolved Hide resolved
Comment on lines +87 to +89
If you don't want to depend on host's libraries versions, or your host is not an **Alpine Linux**/**Ubuntu Linux** or you are using the `alpine-make`/`ubuntu-make` container as a replacement for `make`:

you can run the equivalents checks in a container:
Copy link
Member

Choose a reason for hiding this comment

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

I believe that's better to guide the contributor to always use VM and/or the docker images for that, so the formatting will be aligned with what we expect. But, a huge but, currently alpine image lack some tooling like clang-format-12 and ubuntu image is crashing when running check-pr - they are focused to building and release process.

We have some specific docker images for helping in the contribute process as:

image

You might notice that Makefile.man, by instance, has a pattern to run the Dockerfile.man.

Said that, perhaps bringing a Dockerfile.checkers would be the solution here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think I completely understand what you are saying.
Do you say that we should encourage people to run the docker file or container instead of locally?

Also, do you say we should prefer to use the Dockerfiles or that the make file run the docker file?

Copy link
Member

Choose a reason for hiding this comment

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

Do you say that we should encourage people to run the docker file or container instead of locally?

Yep, as it avoids conflicts with the contributor's host system.

Also, do you say we should prefer to use the Dockerfiles or that the make file run the docker file?

Makefile should run docker under the hood for us. As it does in protoc, man rules etc.

@ShohamBit ShohamBit requested a review from geyslan December 16, 2024 15:13
@@ -120,3 +120,9 @@
```bash
DEBUG=1 make
```

9. Build with embedded metrics (pprof) by setting `METRICS=1`
Copy link
Member

@geyslan geyslan Dec 16, 2024

Choose a reason for hiding this comment

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

Suggested change
9. Build with embedded metrics (pprof) by setting `METRICS=1`
9. Build enabling BPF metrics by setting `METRICS=1`.
BPF metrics are only available if the BPF object is built with `METRICS` debug flag defined.

Just a suggestion to explain what it really enables:

#ifdef METRICS

Since we avoid having code in BPF as much as possible to reduce cpu time, metrics coming from it are disabled by default until requested otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update docs/contributing
2 participants