Install via makefile #57

sloum · 2019-10-15T03:32:49Z

sloum commented

2019-10-15 03:32:49 +00:00

This is a first draft of a makefile based approach. To do so I had to set up a few variables and reference them in the codebase.

I also switched out the help path and default home to the new gopher page's user-guide page.

This is a first draft of a makefile based approach. To do so I had to set up a few variables and reference them in the codebase. I also switched out the help path and default home to the new gopher page's user-guide page.

sloum added the

enhancement

label 2019-10-15 03:33:00 +00:00

asdf was assigned by sloum

2019-10-15 03:33:04 +00:00

sloum commented

2019-10-15 16:22:54 +00:00

I had an additional build flag "CONF_PATH" to try to help accomodate Ben from tilde.team/tilde.club and it had started to gather a bunch of logi around it in the program init process... but then I realized we already provide this functionality by way of defaults.go. This file is meant to be edited by administrators wanting to set defaults for their users. I added some extra comments to help clarify. Before release of v2.0.0 I will want to rewrite the README with a greater focus on the compilation options, configuration, using the makefile, etc.

I think I may have worked out a way for the open in browser stuff to work in a terminal only environment as well. But that will be for a different PR.

The makefile seems to work well so far. It installs and uninstalls the man page cleanly along with the main program, it cleans up build files well, and it can take the option BUILD_PATH to install elsewhere:

make install BUILD_PATH=/usr/local/bin

If BUILD_PATH is not set in this way, it will use the $GOPATH or $GOBIN values to construct the right path. If those are absent it will use the default go location of ~/go/bin.

I had an additional build flag "CONF_PATH" to try to help accomodate Ben from tilde.team/tilde.club and it had started to gather a bunch of logi around it in the program init process... but then I realized we already provide this functionality by way of `defaults.go`. This file is meant to be edited by administrators wanting to set defaults for their users. I added some extra comments to help clarify. Before release of v2.0.0 I will want to rewrite the README with a greater focus on the compilation options, configuration, using the makefile, etc. I think I may have worked out a way for the open in browser stuff to work in a terminal only environment as well. But that will be for a different PR. The makefile seems to work well so far. It installs and uninstalls the man page cleanly along with the main program, it cleans up build files well, and it can take the option `BUILD_PATH` to install elsewhere: ```make install BUILD_PATH=/usr/local/bin``` If BUILD_PATH is not set in this way, it will use the `$GOPATH` or `$GOBIN` values to construct the right path. If those are absent it will use the default go location of `~/go/bin`.

sloum commented

2019-10-17 20:26:09 +00:00

I have discovered that doing the following works well on most systems, particularly for multi-user installs:

sudo make install BUILD_PATH=/usr/local/bin

However, if you just run the following the manpage will be unable to install (it requires sudo), but the binary will install to the default location ($GOBIN). This is a bit of a bummer, but is still functional.

make install

Lastly, if you run the following you get an opposite issue where the manpage installs and the binary installs... but you still cannot run the program because it is in the root user's home gopath.

sudo make install

[The above was tested on Ubuntu]

I am starting to feel like maybe having
/urs/local/bin as the default install path (thus requiring sudo privileges) might be the better way. If that ended up the case, I would provide a make target to do a local install for a single user (without sudo) that adds a /home/share/man/man1/ directory to the user's manpath and installs the manual there.

If this gets chosen there would basically be two major options:

make install

and

make user-install

(or somesuch)

Thoughts?

I have discovered that doing the following works well on most systems, particularly for multi-user installs: ``` sudo make install BUILD_PATH=/usr/local/bin ``` However, if you just run the following the manpage will be unable to install (it requires `sudo`), but the binary _will_ install to the default location (`$GOBIN`). This is a bit of a bummer, but is still functional. ``` make install ``` Lastly, if you run the following you get an opposite issue where the manpage installs _and_ the binary installs... but you still cannot run the program because it is in the root user's home gopath. ``` sudo make install ``` _[The above was tested on Ubuntu]_ - - - I am starting to feel like maybe having `/urs/local/bin` as the _default_ install path (thus requiring `sudo` privileges) might be the better way. If that ended up the case, I would provide a make target to do a local install for a single user (without sudo) that adds a `/home/share/man/man1/` directory to the user's manpath and installs the manual there. If this gets chosen there would basically be two major options: ``` make install ``` and ``` make user-install ``` (or somesuch) - - - Thoughts?

asdf commented

2019-10-19 03:30:57 +00:00

I notice you're using install and not go install - was this approach more flexible for having a configurable install location?

I notice you're using `install` and not `go install` - was this approach more flexible for having a configurable install location?

sloum commented

2019-10-19 04:18:00 +00:00

Yes. Go install will always install to $GOPATH/bin, or at least that seems to be the case. So instead we build in the local folder and then install to a location, then clean up the build files.

Yes. Go install will always install to `$GOPATH/bin`, or at least that seems to be the case. So instead we build in the local folder and then install to a location, then clean up the build files.

asdf commented

2019-10-20 04:36:51 +00:00

I've had a bit more time on this now, and I really like the work you have done. It's modular and easy to understand, and it works well from what I have tested.

A few additional points:

I understand your point about using /usr/local/bin instead of any of the Go paths. Setting make install to use /usr/local/bin would probably align better with the typical use case of an admin performing an install.
In my experience, the default target, or what happens when you just run make, typically just compiles/builds. I'm not sure installing should be the default behaviour here.
There are some conventions about the environment variable names. @ben mentioned $PREFIX, and I also saw some discussion mentioning this as well as an example makefile and an associated config file. So maybe:
Set the variables as $PREFIX = /usr/local and $MANPREFIX = ${PREFIX}/share/man
When installing the binary, install to $PREFIX/bin
When installing the man page, install to $MANPREFIX/man1
I like the idea for the user installation. Should a check be performed to ensure the binary is installed to a directory in $PATH?

I've had a bit more time on this now, and I really like the work you have done. It's modular and easy to understand, and it works well from what I have tested. A few additional points: 1. I understand your point about using `/usr/local/bin` instead of any of the Go paths. Setting `make install` to use `/usr/local/bin` would probably align better with the typical use case of an admin performing an install. 1. In my experience, the default target, or what happens when you just run `make`, typically just compiles/builds. I'm not sure installing should be the default behaviour here. 1. There are some conventions about the environment variable names. @ben mentioned `$PREFIX`, and I also saw [some discussion](https://lobste.rs/s/uq6nfz/what_can_software_authors_do_help_linux) mentioning this as well as an example [makefile](https://git.suckless.org/dmenu/file/Makefile.html) and an associated [config file](https://git.suckless.org/dmenu/file/config.mk.html). So maybe: 1. Set the variables as `$PREFIX = /usr/local` and `$MANPREFIX = ${PREFIX}/share/man` 1. When installing the binary, install to `$PREFIX/bin` 1. When installing the man page, install to `$MANPREFIX/man1` 1. I like the idea for the user installation. Should a check be performed to ensure the binary is installed to a directory in $PATH?

sloum commented

2019-10-20 05:44:07 +00:00

The only problem with using prefix in that way is if they set a path using BUILD_PATH, but I suppose that could be written:

BUILD_PATH ?= ${PREFIX}/bin and that mostly solves that issue.

We could try to check the path. But then what happens if it is not on the path? Do we start writing backup logic? Or just error out (Im not sure how to do that in a makefile).

Definitely good suggestions. I'll work them in when I get some downtime; or if you want, feel free to commit to the branch and just ping me here to check out any changes (I'll do the same if I get to it first).

The only problem with using prefix in that way is if they set a path using `BUILD_PATH`, but I suppose that could be written: `BUILD_PATH ?= ${PREFIX}/bin` and that mostly solves that issue. We could try to check the path. But then what happens if it is not on the path? Do we start writing backup logic? Or just error out (Im not sure how to do that in a makefile). Definitely good suggestions. I'll work them in when I get some downtime; or if you want, feel free to commit to the branch and just ping me here to check out any changes (I'll do the same if I get to it first).

asdf commented

2019-10-21 02:55:32 +00:00

I've pushed a commit with some of the changes I suggested (if this wasn't the right process, let me know).

Sorry for the confusion about BUILD_PATH. I was suggesting replacing BUILD_PATH and man1dir with the PREFIX and MANPREFIX names as per convention, and have done so with this commit.
${PREFIX} can now be used for choosing an install location that isn't /usr/local.
${MANPREFIX} is also available.
${DESTDIR} has also been specified as it is meant to help with packaging - this is something that might be useful in the future, but could be considered for removal at this stage.
Removed the uninstall variable, as it seemed not specific enough. It would be possible to remove another binary called bombadillo that was already in the path. This now uninstalls only what the Makefile would have installed. Happy to reverse this as we did not discuss it earlier.
Running make will now just build Bombadillo, and not proceed with an install unexpectedly. make install must now be used.

Two other points:

I'm not sure that setting the path or manpath for installs to non-path areas is something the makefile should do. It seems like a fair amount of reach in to system/user configuration. One other way of doing a user install could be to document it, something like:
Run make PREFIX=~/local
Ensure ~/local/bin is in your path (hyperlink to some documentation)
Ensure ~/local/share/man is in your manpath (hyperlink to some other documentation)
Running bombadillo -v shows the version only, but we are capturing build time too. I'm not sure if you want to add this to bombadillo, or consider removing it from the makefile.

I've pushed a commit with some of the changes I suggested (if this wasn't the right process, let me know). 1. Sorry for the confusion about `BUILD_PATH`. I was suggesting replacing `BUILD_PATH` and `man1dir` with the `PREFIX` and `MANPREFIX` names as per convention, and have done so with this commit. 1. `${PREFIX}` can now be used for choosing an install location that isn't /usr/local. 1. `${MANPREFIX}` is also available. 1. `${DESTDIR}` has also been specified as it is meant to help with packaging - this is something that might be useful in the future, but could be considered for removal at this stage. 1. Removed the uninstall variable, as it seemed not specific enough. It would be possible to remove another binary called bombadillo that was already in the path. This now uninstalls only what the Makefile would have installed. Happy to reverse this as we did not discuss it earlier. 1. Running `make` will now just build Bombadillo, and not proceed with an install unexpectedly. `make install` must now be used. Two other points: 1. I'm not sure that setting the path or manpath for installs to non-path areas is something the makefile should do. It seems like a fair amount of reach in to system/user configuration. One other way of doing a user install could be to document it, something like: 1. Run `make PREFIX=~/local` 1. Ensure ~/local/bin is in your path (hyperlink to some documentation) 1. Ensure ~/local/share/man is in your manpath (hyperlink to some other documentation) 1. Running `bombadillo -v` shows the version only, but we are capturing build time too. I'm not sure if you want to add this to bombadillo, or consider removing it from the makefile.

sloum commented

2019-10-21 03:32:16 +00:00

I am new to makefiles, so this is all good info. You definitely did the right thing committing to the branch.

I still do not quite understand why people do make && make install when the install could just do the build as part of it... but if that is what people that are used to working with make expect then I guess it mostly makes sense to do it (even if it feels needless to me).

I suppose people still do also have the option of just running go install if they want an installation local to their user (they just wont have the manfile).

This all looks good. I had planned on adding build time to the -v output, but really I could go either way. I'd be fine removing it.

I'll want to update the readme, but there will be more than just makefile stuff to update there, so maybe as a separate pull? I'm good with this one as is if you want to merge it in.

I am new to makefiles, so this is all good info. You definitely did the right thing committing to the branch. I still do not quite understand why people do `make && make install` when the install could just do the build as part of it... but if that is what people that are used to working with make expect then I guess it mostly makes sense to do it (even if it feels needless to me). I suppose people still do also have the option of just running `go install` if they want an installation local to their user (they just wont have the manfile). This all looks good. I had planned on adding build time to the -v output, but really I could go either way. I'd be fine removing it. I'll want to update the readme, but there will be more than just makefile stuff to update there, so maybe as a separate pull? I'm good with this one as is if you want to merge it in.

ben commented

2019-10-21 13:28:43 +00:00

First-time contributor

It's possible to just run make install but specifying it separately emphasizes the ability to change build options before installing. It's a convention that may or may not apply here.

It's possible to just run `make install` but specifying it separately emphasizes the ability to change build options before installing. It's a convention that may or may not apply here.

sloum commented

2019-10-21 15:45:32 +00:00

@ben Thanks for the info! Not coming from a C background this is all new to me :) I appreciate y'all s help

asdf commented

2019-10-22 03:17:30 +00:00

Yeah thanks @ben for the info! I had wondered about that too. If you do get a chance to check out how the makefile works on your systems, let us know how you go. Feel free to give us any other feedback you might have.

@sloum I've made some changes to README.md to reflect the new functionality in this PR. As you suggest, other changes might be incorporated in to a new PR (but I did change the Prerequisites section...a bit out of scope...)

Yeah thanks @ben for the info! I had wondered about that too. If you do get a chance to check out how the makefile works on your systems, let us know how you go. Feel free to give us any other feedback you might have. @sloum I've made some changes to README.md to reflect the new functionality in this PR. As you suggest, other changes might be incorporated in to a new PR (but I did change the Prerequisites section...a bit out of scope...)

sloum commented

2019-10-22 05:32:24 +00:00

@asdf Looks good :) I reviewed the other PR you opened ( #59 ). That one and this one seem to now match in content and I'm having trouble telling what is different about them. Are there unique things between them that I am missing?

asdf commented

2019-10-23 02:35:01 +00:00

Ah whoops looks like this might be a bit messy then! Sorry for the confusion.

The items you've reviewed in #59 are from this PR.

PR #59 is based on this branch, because it builds upon these changes. It targets develop, which does not yet have these changes, so all these commits were also included.

I thought that once this PR was merged, it would be easier to understand #59. Maybe I should have waited before creating the PR for #59, or perhaps instead indicate it is on hold?

In any case, I think it's best to get this one in before any further action on #59. I'll respond to your review items here.

Ah whoops looks like this might be a bit messy then! Sorry for the confusion. The items you've reviewed in #59 are from this PR. PR #59 is based on this branch, because it builds upon these changes. It targets develop, which does not yet have these changes, so all these commits were also included. I thought that once this PR was merged, it would be easier to understand #59. Maybe I should have waited before creating the PR for #59, or perhaps instead indicate it is on hold? In any case, I think it's best to get this one in before any further action on #59. I'll respond to your review items here.

sloum reviewed 2019-10-23 05:24:19 +00:00

sloum left a comment

Minus the isntallation step issue that I commented on, I think this is good. I'm marking it as approved, pending a brief check on the installation logic/flow per the comment. IF that gets either fixed or is shown to not be an issue feel free to merge in :)

README.md

						
				@ -21,3 +21,3 @@

				git clone https://tildegit.org/sloum/bombadillo.git

				cd bombadillo

				go install

				sudo make install

sloum commented

2019-10-23 05:21:16 +00:00

This should be:

git clone https://tildegit.org/sloum/bombadillo.git
cd bombadillo
make
sudo make install

If they do not run make before sudo make install then there will be no binary to install as part of the install step.

This should be: ``` git clone https://tildegit.org/sloum/bombadillo.git cd bombadillo make sudo make install ``` If they do not run `make` before `sudo make install` then there will be no binary to install as part of the install step.

asdf approved these changes 2019-10-23 05:44:06 +00:00

README.md

						
				@ -21,3 +21,3 @@

				git clone https://tildegit.org/sloum/bombadillo.git

				cd bombadillo

				go install

				sudo make install

asdf commented

2019-10-23 02:44:42 +00:00

running make before make install is not required.

The install target depends on install-bin, and install-bin depends on build (see line 32 of the Makefile). So, the build target is run whenever you run make install.

If you prefer the idea of make then make install I can update this.

running `make` before `make install` is not required. The `install` target depends on `install-bin`, and `install-bin` depends on `build` (see line 32 of the Makefile). So, the `build` target is run whenever you run `make install`. If you prefer the idea of `make` then `make install` I can update this.

sloum commented

2019-10-23 13:48:01 +00:00

Nope. That was my bad. I realize that now as obvious. I only looked at install and saw that it didnt not call build. But forgot to look to see if intall-bin did.

README.md

						
				@ -32,3 +44,3 @@

				#### Troubleshooting

				If you run `bombadillo` and get `bombadillo: command not found`, try running `go build` from within the cloned repo. Then try: `./bombadillo`. If that works it means that Go does not install to your path. `go build` added an executable file to the repo directory. Move that file to somewhere on your path. I suggest `/usr/local/bin` on most systems, but that may be a matter of personal preference.

				If you run `bombadillo` and get `bombadillo: command not found`, try running `make` from within the cloned repository. Next, try: `./bombadillo`. If this works, it means that the installation was not completed to an area in your `PATH`.

asdf commented

2019-10-23 02:47:32 +00:00

It is required to run make here to ensure the binary is available.

If we assume the user has already run make install, the last dependency of install is clean which removes the binary. So it will not be in the directory when the user gets to this step.

It is required to run `make` here to ensure the binary is available. If we assume the user has already run `make install`, the last dependency of `install` is `clean` which removes the binary. So it will not be in the directory when the user gets to this step.

sloum commented

2019-10-23 13:49:08 +00:00

Yup. It all makes sense to me now (it is morning and I am more awake, lol).

asdf commented

2019-10-23 05:45:23 +00:00

Sorry I had made some comments from your other review, but didn't know I had to approve them for you to see them. Mainly just my response to the install instructions query.