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

[DNM]Using bazel as building tool #3539

Closed
wants to merge 21 commits into from

Conversation

zhexuany
Copy link
Contributor

@zhexuany zhexuany commented Jun 24, 2017

This change is Reviewable

@zhexuany zhexuany force-pushed the using_bazel_as_building_tool branch from 493578e to df028d2 Compare June 24, 2017 03:31
@zhexuany
Copy link
Contributor Author

@shenli
You can compare normal make build and bazel build.

bazel build //tidb-server/...

@zhexuany zhexuany changed the title [WIP]Using bazel as building tool Using bazel as building tool Jun 25, 2017
@zhexuany zhexuany force-pushed the using_bazel_as_building_tool branch from b81a35b to 5ac15c3 Compare June 26, 2017 02:52
@zhexuany
Copy link
Contributor Author

@ngaut @shenli

PR is ready to review.

What do I propose in this PR?

  1. Kubernetes already adopts bazel as their automated tool. It will be good if we take the same action. You can find more detail in this. bazel is typically good for large code base like us. It provides correctness and scalability on contrast go can not provides these.

  2. Another change I proposed in this PR is seperate version from util/printer.

What is the benefit of these proposals?

  1. Bazel build can accelerate incremental build. For normal build, bazel may be slower than normal make build. The reason behind this is that bazel need do dependent analysis which is beneficial in future.

     Normal Build result with command `time make`:
     	make  46.23s user 4.62s system 341% cpu 14.895 total
     Bazel build resut with command `bazel build //tidb-server/...`:
     	INFO: Found 2 targets...
     	INFO: Elapsed time: 35.095s, Critical Path: 30.82s
    

    If you just change one line in your code base, and recompile it. Normal make build still need consume at least 15 seconds. But bazel is much fater. We use bazel build again and the result is:

     INFO: Found 2 targets...
     INFO: Elapsed time: 3.505s, Critical Path: 1.87s
    

    Even more, bazel can actually cached test output. When you run bazel test, it will only trigger test if such package has changes which can improve our developing efficiency.

  2. version changes.

    Originally, TiDB only has TiDBBuildTS and TiDBGitHash defined in util/printer. I made a new directory version and related shell scripts in this PR. shell scripts extract version information from git tree automatically and write it into ldflags. This feature will be usefull once we enter GA era. I will create release.sh in nearly furture. This shell script can create release iteself according to the info you passed into.

I recommnad we maintain two build system until all developers are familar with bazel.

make will trigger bazel build and make normal_build will trigger normal make build.

@shenli
Copy link
Member

shenli commented Jun 26, 2017

@zhexuany OK I will take a look.

@zhexuany
Copy link
Contributor Author

There is one workaround in this PR. plan and executor are circular dependency. For resolving this, a init function was added in executor. This is just fine since go test will do the sharing job. plan.EvalSubquery is initialized in executor. But rules_go is not implemented this functionality yet. We have to wait or I will implement it if I have time.

@unship
Copy link
Contributor

unship commented Jun 26, 2017

change

bazel-test:
	bazel test //executor/... //distsql/... //mysql/... //sessionctx/...  --test_output=errors
	bazel test //ddl/...
	bazel test //terror/... //ast/... //infoschema/... //parser/... //statistics/...  --test_output=errors
	bazel test //domain/... //inspectkv/... //perfschema/... //store/... //util/...  --test_output=errors
	bazel test //cmd/... //kv/... //structure/... //context/... //expression/...  --test_output=errors
	bazel test //meta/... //privilege/... //table/... //model/...  //server/... //tablecodec/...  --test_output=errors

to

bazel-test:
    bazel test //...:all --test_output=errors

could be better?

@unship
Copy link
Contributor

unship commented Jun 26, 2017

load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_prefix", "go_test")

go_prefix("github.com/pingcap/tidb/foo")

go_library(
    name = "go_default_library",
    srcs = glob(["*.go"], exclude=["*_test.go"]),
    visibility = ["//visibility:public"],
)
go_test(
    name = "go_default_test",
    srcs = glob(["*_test.go"]),
    visibility = ["//visibility:public"],
)

as a templet of every go package

make it easy when some add a file to a go package, he do not need to mod the build file, while preserve the sematics

@zhexuany
Copy link
Contributor Author

No need actually. All BUILD.bazel are generated file. Check this.

And for your last suggestion, your solution is great and I already adopt your suggestion. But do you know any way to cached test result?


Review status: 0 of 864 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@unship
Copy link
Contributor

unship commented Jun 26, 2017

just run bazel info, it will tell you the internal temp file location

@unship
Copy link
Contributor

unship commented Jun 26, 2017

bazel build //tidb-server -c dbg will not generate debug info for debuger on mac, same as bazelbuild/bazel#2537,
while make can

screenshot
screenshot

@hanfei1991 hanfei1991 added the WIP label Jul 3, 2017
@shenli shenli changed the title Using bazel as building tool [DNM]Using bazel as building tool Jul 4, 2017
@shenli
Copy link
Member

shenli commented Jul 4, 2017

@zhexuany We will consider this latter.

@shenli shenli closed this Jul 14, 2017
@zhexuany zhexuany deleted the using_bazel_as_building_tool branch January 4, 2018 08:22
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.

4 participants