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

Proposal: Improve Makefile #735

Merged
merged 5 commits into from
Dec 20, 2022
Merged

Conversation

dio
Copy link
Contributor

@dio dio commented Nov 14, 2022

Note: Sorry that I put this proposal as a draft PR instead of an issue since it is easier for me to present the idea via code.

This proposes Makefile improvement. To let the transition smooth, this PR puts the proposal into a new file called: Next.mk. If this is something that we are looking for, we can "replace" the main Makefile.

To invoke a target inside it, we can run it with make -f Next.mk.

This proposal is broken down into three main commits:

  • Add Next.mk 0eae5da, this adds the basic structure.
  • Add test target 6295a15, this adds the test target. Basically runs the bazel test //tests/....
  • Add sanity tests f83fd5a, this adds checks for bazel-related tasks.

In this proposal, the Next.mk has several targets, that can be inspected via the "help" target.

image

Next.mk depends on several *.mk files:

  • Tools.mk provides dependency tools versions.
  • tools/build/Help.mk provides the "help" target.
  • tools/build/Env.mk sets some required env vars.
  • tools/build/Installer.mk provides targets that install tools. This is done on-demand.

Some quick notes:

  1. This proposal moves the top BUILD file to BUILD.bazel hence "build" directory can be used to store the current plugin binary.
  2. This proposal also adds a new workflow: .github/workflows/commit.yaml.
  3. To run "build" and "check". The build step runs clean before
    build hence the *.pb.go is regenerated and its consistency can be
    checked via make check.
  4. Some packaging helpers are not yet included in this proposal since we do packaging via workflow and not calling the make targets.

Next

After this, the plan is to:

  1. Do hygiene and code-quality tasks, by adding format and lint targets. Also, a way to measure the coverage. We can add the format and lint check here, but that will actually introduce more file changes due to that (hence the PR gets bigger and will be hard to review).
  2. Fresh up the bazel/*. Seems like at least we can (the work on this is already started):
    a. Upgrade protobuf dependency to match the one with non-bazel, and use compiled protoc whenever possible to reduce the time cost (similar to what we have with the envoy project).
    b. Use @maven via https://github.com/bazelbuild/rules_jvm_external.
    c. Make sure the python version and modules we use are pinned.

Signed-off-by: Dhi Aurrahman dio@rockybars.com

This proposes a new Makefile. Currently, it resides in Next.mk.
To invoke a target inside it, run it with `make -f Next.mk`.

The Next.mk has several targets, that can be inspected via the
help target.

```
$ make -f Next.mk help
protoc-gen-validate
build      Build the plugin
clean      Clean all build and test artifacts
check      Verify contents of last commit
help       Describe how to use each target
```

Next.mk depends on several *.mk files:
- Tools.mk provides dependency tools versions.
- tools/build/Help.mk provides the "help" target.
- tools/build/Env.mk sets some required env vars.
- tools/build/Installer.mk provides targets that install tools.

This patch moves BUILD to BUILD.bazel hence "build" directory
can be used to store the current plugin binary.

This patch also adds a new workflow: .github/workflows/commit.yaml
To run "build" and "check". The build step runs clean before
build hence the *.pb.go is regenerated and its consistency can be
checked via `make check`.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
This adds the test target to run the required tests. The test
target can be invoked via: `make -f Next.mk test`. This target
runs test via bazel.

The bazel command is provided via bazelisk so we can pick up
the right bazel version (hint: .bazelversion).

Before running the test, we generate the required *.go files
via tests_harness_cases_go (which is a dependency of test target).

This patch adds test target invocation in the workflow, also
sets up the corresponding caching mechanism for bazel.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
This runs bazel-related checks.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio
Copy link
Contributor Author

dio commented Nov 14, 2022

The "CI-runs" on my "fork" can be observed from: dio#2. 🙇🏾

@dio
Copy link
Contributor Author

dio commented Nov 15, 2022

@elliotmjackson do you think this makes sense? Thank you!

@elliotmjackson
Copy link
Contributor

yes, it absolutely makes sense - I just want some time to take a look at the github action changes. Right now, im a touch busy but i will look at this soon

@dio
Copy link
Contributor Author

dio commented Nov 17, 2022

Thanks, @elliotmjackson. Sure, take your time.

@elliotmjackson
Copy link
Contributor

Im quite happy with the changes to the makefile, they are very much needed and this is a good first step. My concerns relate to this new github action, its run time far exceeds our newly tweaked CI and somewhat undoes a lot of the gains we have achieved.

@dio
Copy link
Contributor Author

dio commented Dec 4, 2022

@elliotmjackson seems like we need to keep the current one and iterate on the new one. I'll take a deeper look at the disparity list.

Also, if you can expand on the following points:

  • its run time far exceeds our newly tweaked CI (do you mean it is too slow?)
  • somewhat undoes a lot of the gains we have achieved.

That would be great.

@dio
Copy link
Contributor Author

dio commented Dec 4, 2022

Ah yeah, it is too slow, will take a look (seems like we don't have to do all the stuff "sequentially" and via bazel).

The current one is cached bazel, I think when we have this cached (the bazel stuff), it will be better.

@elliotmjackson
Copy link
Contributor

Ok cool, I assume caching only happens from the base branch. Happy to see where it goes... thanks for the contribution @dio and I apologise for not being overly focused on this repo to date, will endeavour to give you more attention next time.

@dio
Copy link
Contributor Author

dio commented Dec 20, 2022

@elliotmjackson no worries at all :). Love to contribute more if you can help me on merging this PR. Thanks!

@dio
Copy link
Contributor Author

dio commented Dec 20, 2022

(sorry was out too for personal reason)

@elliotmjackson elliotmjackson merged commit 0bd3773 into bufbuild:main Dec 20, 2022
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.

None yet

2 participants