Fix Makefile as per #66 #76

asdf · 2019-11-04T03:21:43Z

asdf commented

2019-11-04 03:21:43 +00:00

Updated Makefile as requested for #66

The install step for the binary has changed too. Previously it was like install binary /path/to/binary but now it is install binary /path/to which seems more correct?

README.md will probably need to be amended too, but maybe only to suggest PREFIX instead of DESTDIR for the custom install, and to check the Makefile for further customisation options. But please let me know what you think about this.

Updated Makefile as requested for #66 The install step for the binary has changed too. Previously it was like `install binary /path/to/binary` but now it is `install binary /path/to` which seems more correct? README.md will probably need to be amended too, but maybe only to suggest PREFIX instead of DESTDIR for the custom install, and to check the Makefile for further customisation options. But please let me know what you think about this.

asdf added this to the 2.0.0 milestone 2019-11-04 03:21:43 +00:00

asdf self-assigned this 2019-11-04 03:21:43 +00:00

sloum was assigned by asdf

2019-11-04 03:21:43 +00:00

asdf added the

non-code

non-functional

build

labels 2019-11-04 03:21:43 +00:00

sloum approved these changes 2019-11-04 05:45:58 +00:00

sloum left a comment

This looks good. I believe, though am not certain, that if BINDIR is set by ?= that gives flexibility for someone to override, which is my desired outcome (I think people should be able to install to wherever they want, with reasonable defaults). That said, I am still shaky on the reasons behind makefile conventions.

This looks good. I believe, though am not certain, that if `BINDIR` is set by `?=` that gives flexibility for someone to override, which is my desired outcome (I think people should be able to install to wherever they want, with reasonable defaults). That said, I am still shaky on the reasons behind makefile conventions.

Makefile Outdated

						
				@ -4,0 +3,4 @@

				EXEC_PREFIX := ${PREFIX}

				BINDIR := ${EXEC_PREFIX}/bin

				DATAROOTDIR := ${PREFIX}/share

				MANDIR := ${DATAROOTDIR}/share/man

sloum commented

2019-11-04 05:43:02 +00:00

If DATAROOTDIR is $prefix/share and MANDIR is $datarootdir/share/man wont that lead to /usr/local/share/share/man? I think MANDIR should not have /share in it, unless I am thinking about this incorrectly.

If `DATAROOTDIR` is `$prefix/share` and `MANDIR` is `$datarootdir/share/man` wont that lead to `/usr/local/share/share/man`? I think MANDIR should not have `/share` in it, unless I am thinking about this incorrectly.

asdf commented

2019-11-04 20:57:46 +00:00

No, that was a mistake. Thanks for picking that up!

asdf commented

2019-11-04 22:11:38 +00:00

Yeah I do have to say that I'm finding make confusing, or maybe just too permissive or non-specific or ambiguous...that's a beginner's perspective though.

The variables we have set can be overridden from the command line, and the assignment operator has no impact in our case. See Overriding in the manual.

With this many variables, there are a lot of options available to people to customise their install, and it should be pretty standard. I think the most usable options are PREFIX as a convention, or BINDIR and MAN1DIR for complete control. I don't really know though, so let me know if you have other ideas on this. I'll update the readme accordingly.

Yeah I do have to say that I'm finding make confusing, or maybe just too permissive or non-specific or ambiguous...that's a beginner's perspective though. The variables we have set can be overridden from the command line, and the assignment operator has no impact in our case. See [Overriding](https://www.gnu.org/software/make/manual/make.html#Overriding) in the manual. With this many variables, there are a lot of options available to people to customise their install, and it should be pretty standard. I think the most usable options are PREFIX as a convention, or BINDIR and MAN1DIR for complete control. I don't really know though, so let me know if you have other ideas on this. I'll update the readme accordingly.

sloum commented

2019-11-05 01:35:55 +00:00

Yeah, I also find it strangely loose and a bit confusing.

So long as those can still be set then I am fine with this as is/it looks good. It is strange that they have an assignment operator ?= that sets something only if it has not already been set, if you can just override anyway, lol. Thanks for the link, it was helpful. There are so many docs for make I feel like I read some and get a handle on things, but then it is blown out of the water by more doc reading.

I approve this pull :) Thanks for putting it together!

Yeah, I also find it strangely loose and a bit confusing. So long as those can still be set then I am fine with this as is/it looks good. It is strange that they have an assignment operator `?=` that sets something only if it has not already been set, if you can just override anyway, lol. Thanks for the link, it was helpful. There are so many docs for make I feel like I read some and get a handle on things, but then it is blown out of the water by more doc reading. I approve this pull :) Thanks for putting it together!

asdf commented

2019-11-05 03:31:48 +00:00

Some small final changes - corrected the error in the makefile that you pointed out. I've also amended the README slightly, and removed some text that was erroneously included earlier.

asdf closed this pull request

2019-11-05 03:32:05 +00:00

sloum deleted branch extend-makefile

2019-11-05 05:33:40 +00:00

Sign in to join this conversation.

No reviewers