-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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 build file to run with an argument #14097
Conversation
The current Makefile doesn't allow the compilation of the tools directory. This commit updates the Makefile and the build file to add an option to build the tools. To build the tools, you can run make build_tools.
Codecov Report
@@ Coverage Diff @@
## main #14097 +/- ##
==========================================
- Coverage 75.18% 74.95% -0.23%
==========================================
Files 452 452
Lines 36769 36769
==========================================
- Hits 27645 27562 -83
- Misses 7388 7459 +71
- Partials 1736 1748 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Overall the directions looks good to me.
Ideally we should separate build.sh
into build_lib.sh
that is used by test.sh and separate build_tools.sh
& build_etcd.sh
top-level script.
Makefile
Outdated
@@ -27,6 +27,9 @@ build: | |||
./bin/etcdctl version | |||
./bin/etcdutl version | |||
|
|||
build_tools: | |||
GO_BUILD_FLAGS="-v" ./scripts/build.sh tools_build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you, please change it to use build_tools
across the board (including test.sh) script. Otherwise it will be confusing to remember whether it's tools_build
or build_tools
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ptabor Thank you for your feedback. I will change to build_tools
to make it consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that the tools_build
function in the build.sh was already there. It simply didn't have a way to be called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reviewing the function names in the build library functions, I saw they are called etcd_build
, tools_build
, test_build
, so for consistency I recommend we keep the tools_build
name. I hope that's OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding creating a build_etcd.sh
script, the current build.sh
is referred to many places, so I suggest we keep it for consistency. Now a build_etcd.sh
file would be almost a duplicate of build.sh
. Do you still want to have it created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Let's keep build.sh. Thank you for the change.
The current Makefile doesn't allow the compilation of the tools directory. This commit creates a build library file, updates the Makefile and a top level script fod building tools. To build the tools, you can run make build_tools. As before, you can run make build to build etcd binaries.
LGTM. |
The current Makefile doesn't allow the compilation of the tools directory.
This commit updates the Makefile and the build file to add an option
to build the tools.
To build the tools, you can run
make build_tools
.