Use new C++ base image that removes local from the directory layout. #4500
Conversation
jcferretti
requested review from
chipkent,
alexpeters1208,
rcaudy,
jmao-denver and
devinrsmith
as code owners
| 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.
you can also say make VERBOSE=1... maybe that's simpler or it doesn't matter.
There was a problem hiding this comment.
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.
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.
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.
We agreed on a check of the var + fail instead of default.
jcferretti
added
NoDocumentationNeeded
NoReleaseNotesNeeded
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.