Makefile: fix DESTDIR environment variable behaviour#5535
Makefile: fix DESTDIR environment variable behaviour#5535estesp merged 1 commit intocontainerd:masterfrom
Conversation
|
Hi @metux. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Build succeeded.
|
AkihiroSuda
left a comment
AkihiroSuda
left a comment
There was a problem hiding this comment.
Please add a documentation to explain that the behavior of DESTDIR was changed in this release (v1.6).
DESTDIR is used by scripts and downstream makefiles that we use for local and release installs. There are some release overrides for DESTDIR in the makefile.. and some missing for the local install-deps where the scripts are expecting either a local destdir that includes prefix.. or a override destdir for release... I like the idea of documenting in |
where / how exactly ? |
Just adding comment lines to Makefile would be fine. |
metux
force-pushed
the
submit/makefile-destdir
branch
2 times, most recently
from
8e7acc0 to
e0df417
Compare
There was a problem hiding this comment.
Could you add the following texts
The files will be installed under `$(DESTDIR)/$(PREFIX)`.
The convention of `DESTDIR` was changed in containerd v1.6.
There was a problem hiding this comment.
Could you add the following texts
The files will be installed under `$(DESTDIR)/$(PREFIX)`. The convention of `DESTDIR` was changed in containerd v1.6.
Okay. Directly in the makefile ? Line 21 ?
There was a problem hiding this comment.
Yes, directly in the makefile around L21 would be fine.
There was a problem hiding this comment.
Puhsed. I've put to above definition of PREFIX.
|
Build succeeded.
|
metux
force-pushed
the
submit/makefile-destdir
branch
from
e0df417 to
9b3821d
Compare
alakesh
left a comment
alakesh
left a comment
There was a problem hiding this comment.
LGTM if suggested changes to documentation is done.
|
Build succeeded.
|
The DESTDIR environment variable is used in the wrong way: since 30+ years
it's common practise using it for specifying *temporary* target *root*
(where eg. packaging infrastructure picks up the install image), instead
of the installation *prefix* on the final target.
Fixing this by introducing PREFIX variable (with default /usr/local) and
using both variables according to the 30 years matured common practise.
That way, distro packagers and other standardized build/installation
systems can easily do correct deployments w/o individual hacks.
changes v2: removed variables not used yet
added documentation
Signed-off-by: Enrico Weigelt, metux IT consult <[email protected]>
metux
force-pushed
the
submit/makefile-destdir
branch
from
9b3821d to
b5f530a
Compare
|
/ok-to-test |
|
Build succeeded.
|
|
/test pull-containerd-node-e2e |
9 participants
The DESTDIR environment variable is used in the wrong way: since 30+ years
it's common practise using it for specifying temporary target root
(where eg. packaging infrastructure picks up the install image), instead
of the installation prefix on the final target.
Fixing this by introducing PREFIX variable (with default /usr/local) and
using both variables according to the 30 years matured common practise.
That way, distro packagers and other standardized build/installation
systems can easily do correct deployments w/o individual hacks.
Signed-off-by: Enrico Weigelt, metux IT consult [email protected]