Skip to content
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

allow installation in nonstandard directories #136

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

allow installation in nonstandard directories #136

wants to merge 2 commits into from

Conversation

GitMensch
Copy link

GitMensch added 2 commits May 4, 2018 13:22
Renamed "Quick Start" to "Installation", added the `make install` part and a hint for redirection - nearly from https://raw.githubusercontent.com/lz4/lz4/dev/README.md
DESTDIR ?=
# directory variables : GNU conventions prefer lowercase
# see https://www.gnu.org/prep/standards/html_node/Makefile-Conventions.html
# support both lower and uppercase (BSD), use uppercase in script
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You introduced the uppercase variables, but didn't actually use them in the install target below

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevengj You are right. I see the reasoning of introducing the uppercase variables, but it worked as-is (on SLES).
Do you prefer to drop the "BSD variant" completely or change the Makefile to use the upper-case variables? I'll commit the appropriate change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better have only the lowercase variants, things get really messy with two variables for each directory.

@stevengj
Copy link
Member

stevengj commented May 4, 2018

Doesn't e.g. make prefix=/foo/bar already work prior to this patch? Setting a variable on the command line should already override that variable in the file even without using ?=.

?= only allows the variable to be overridden by an environment variable, but you can also do that by just running make -e.

@GitMensch
Copy link
Author

GitMensch commented May 4, 2018

Doesn't e.g. make prefix=/foo/bar already work prior to this patch?

No, make prefix=/foo/bar install was actually what I've did - and got an access denied for /usr/something. After using the Makefile including in the PR the install worked.

@nalimilan
Copy link
Member

I'm surprised to hear it doesn't work, as I use make install DESTDIR=%{buildroot} prefix=%{_prefix} includedir=%{_includedir} libdir=%{_libdir} in the Fedora package and it works fine.

@stevengj
Copy link
Member

stevengj commented May 5, 2018

and got an access denied for /usr/something

An "access denied" error has nothing to do with the Makefile. You just forgot to use sudo.

@GitMensch
Copy link
Author

An "access denied" error has nothing to do with the Makefile. You just forgot to use sudo.

No, this has something to do with the Makefile as it was raised when doing

make prefix=/foo/bar install - I did not intended to install anything into system space.

With the changed Makefile the same command worked perfectly fine...

@stevengj
Copy link
Member

stevengj commented May 5, 2018

@GitMensch, what version of make are you using? (What is make -v)

@GitMensch
Copy link
Author

@stevengj

GNU Make 4.2.1
Built for i686-pc-linux-gnu

To finish this PR:
Do you prefer to drop the "BSD variant" (upper-case variables) completely or change the Makefile to use the new defined upper-case variables (that will be set from the upper-case variables when passed to make, otherwise from the lower-case variables when passed to make, otherwise from the old default)?
I'll commit the appropriate change.

@stevengj
Copy link
Member

stevengj commented May 8, 2018

I'm still trying to figure out what problem is being solved here. The current Makefile already has a prefix variable. make prefix=foo should already override this in GNU make. The main difference with ?=, as I understand it, arises if e.g. you include our Makefile in another makefile and you want the former's definition to take precedence, but that isn't the case here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants