-
Notifications
You must be signed in to change notification settings - Fork 80
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
Use new C++ base image that removes local from the directory layout. #4500
Conversation
You can tweak the last command above as | ||
|
||
``` | ||
VERBOSE=1 make -j$NCPUS install 2>&1 | tee make-install.log |
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 can also say make VERBOSE=1
... maybe that's simpler or it doesn't matter.
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.
I feel a bit icky about make VERBOSE=1 -j32
because of the order of definition versus flag, but it happens to work, at least in GNU make, and in practice all these linux distributiosn are GNU.
I believe I prefer the environment variable for the process
regular shell syntax tho, because that's more general.
Do you care if we just leave it as is?
Update: Corey and myself discussed, we are keeping it as is for now.
: ${CPP_PROTO_BUILD_DIR:=build} | ||
|
||
mkdir -p "${CPP_PROTO_BUILD_DIR}" | ||
$DHCPP_LOCAL/protobuf/bin/protoc \ | ||
$DHCPP_LOCAL/bin/protoc \ |
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.
I kind of wonder two things:
- whether this should be DHCPP not DHCPP_LOCAL (i.e. why "local")
- whether this should not be set at all, and we should tell people to use the same $DHCPP variable they got when they ran the "env.sh" file we generated for them
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.
I think you are right about 1, changed and pushed.
Ref 2, there is one level of indirection that is always necessary, something needs to point to where the env.sh file lives; so we need something to know where the $PREFIX deployment was made. The way this code is written, it is essentially assuming a default (which I don't think is most of the time used... this script is meant for manual execution, and you/me should come here with the env.sh file sourced already). The syntax
: ${POPO:=popo_default}
essentially tells the shell that if the POPO
environment variable is already defined, use its value, otherwise if it is not defines, use popo_default
.
So maybe I would question the default and we could change the logic to "check if the variable is not defined, and if it is not, bail with an error".
?
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.
We agreed on a check of the var + fail instead of default.
This simplifies our deployment layout, eliminated one unnecessary level of indirection (the intermediate
.../local/...
directory).The C++ base image being adopted was already modified to do this, and also trim down the number of files
in the final deployment hierarchy; for builds that include debug information, we are still keeping the source files of
all libraries, including dependent, but the object files and in general the intermediate results of compilation are not necessary after install is done, so this trims down our install footprint considerably.