nicolasff / webdis

A Redis HTTP interface with JSON output
https://webd.is
BSD 2-Clause "Simplified" License
2.82k stars 307 forks source link

Do not add slash between between DESTDIR and PREFIX in makefile #172

Closed marsam closed 4 years ago

marsam commented 4 years ago

This change will allow to only use PREFIX and leave DESTDIR empty.

nicolasff commented 4 years ago

Hi @marsam, thanks for the PR! I'm not entirely sure I follow what this allows that didn't work before. If you wanted to install it to a different location, couldn't you just use e.g. this?

DESTDIR=~/webdis PREFIX= make install

This does create ~/webdis/bin/webdis and ~/webdis/etc/webdis.prod.json:

$ DESTDIR=~/webdis-install PREFIX= make install
Makefile:53: target '/home/testuser/webdis-install' given more than once in the same rule
mkdir -p /home/testuser/webdis-install
mkdir -p /home/testuser/webdis-install/bin
mkdir -p /home/testuser/webdis-install/etc
cp webdis /home/testuser/webdis-install/bin
cp webdis.prod.json /home/testuser/webdis-install/etc

Note that this warning is new with this PR and does not occur without the change.

I understand that the idea of this change is that we could swap DESTDIR and PREFIX, but the purpose of DESTDIR is to set the destination directory and it seems to serve this purpose just fine right now.

And actually with this change, it doesn't seem like setting PREFIX but leaving DESTDIR empty works as expected:

$ DESTDIR= PREFIX=~/webdis-install make install
mkdir -p /home/testuser/webdis-install
mkdir -p /home/testuser/webdis-install/bin
cp webdis /home/testuser/webdis-install/bin
cp webdis.prod.json /etc
cp: cannot create regular file '/etc/webdis.prod.json': Permission denied
Makefile:60: recipe for target 'install' failed
make: *** [install] Error 1
nicolasff commented 4 years ago

@marsam any update on the questions above?

marsam commented 4 years ago

Hi, thanks for answering, and sorry for being vague in the PR description.

I understand that the idea of this change is that we could swap DESTDIR and PREFIX, but the purpose of DESTDIR is to set the destination directory and it seems to serve this purpose just fine right now.

DESTDIR is used as poor man's sandbox. In nixpkgs we usually refrain from using DESTDIR, because it might have negative side-effects https://github.com/NixOS/nixpkgs/issues/65718. I'm aware that webdis is not really affected by those, since it only installs the binary, but it would be nice if we (nixpkgs) didn't have to use DESTDIR for consistence' sake.

Removing the extra slash (/) should not cause any regression to your setup, for instance

DESTDIR=~/webdis-install PREFIX= make install

should still work, but with leaving DESTDIR empty you would need to set CONFDIR for make install.

I hope it sounds reasonable.

P.S. Thanks for writing webdis, It's really useful

nicolasff commented 4 years ago

With your change, running the command you just quoted now prints a warning, which was not there before the change:

$ DESTDIR=~/webdis-install PREFIX= make install
-----> Makefile:53: target '/home/testuser/webdis-install' given more than once in the same rule
mkdir -p /home/testuser/webdis-install
mkdir -p /home/testuser/webdis-install/bin
mkdir -p /home/testuser/webdis-install/etc
cp webdis /home/testuser/webdis-install/bin
cp webdis.prod.json /home/testuser/webdis-install/etc
marsam commented 4 years ago

fixed

nicolasff commented 4 years ago

Great, now both of these work as expected: DESTDIR= CONFDIR=~/webdis-install/etc PREFIX=~/webdis-install make install and DESTDIR=~/webdis-install PREFIX= make install

Thanks again!