Install via makefile #57
No reviewers
Labels
No Label
blocked
bug
build
documentation
duplicate
enhancement
finger
gemini
gopher
help wanted
http
in progress
invalid
local
needs-info
non-code
non-functional
non-urgent
question
release
rendering
suggestion
telnet
terminal
urgent
wontfix
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sloum/bombadillo#57
Loading…
Reference in New Issue
No description provided.
Delete Branch "makefile"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
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 have discovered that doing the following works well on most systems, particularly for multi-user installs:
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.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.
[The above was tested on Ubuntu]
I am starting to feel like maybe having
/urs/local/bin
as the default install path (thus requiringsudo
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:
and
(or somesuch)
Thoughts?
I notice you're using
install
and notgo install
- was this approach more flexible for having a configurable install location?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.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:
/usr/local/bin
instead of any of the Go paths. Settingmake install
to use/usr/local/bin
would probably align better with the typical use case of an admin performing an install.make
, typically just compiles/builds. I'm not sure installing should be the default behaviour here.$PREFIX
, and I also saw some discussion mentioning this as well as an example makefile and an associated config file. So maybe:$PREFIX = /usr/local
and$MANPREFIX = ${PREFIX}/share/man
$PREFIX/bin
$MANPREFIX/man1
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).
I've pushed a commit with some of the changes I suggested (if this wasn't the right process, let me know).
BUILD_PATH
. I was suggesting replacingBUILD_PATH
andman1dir
with thePREFIX
andMANPREFIX
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.make
will now just build Bombadillo, and not proceed with an install unexpectedly.make install
must now be used.Two other points:
make PREFIX=~/local
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 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.
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.@ben Thanks for the info! Not coming from a C background this is all new to me :) I appreciate y'all s help
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...)
@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?
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.
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 :)
@ -21,3 +21,3 @@
git clone https://tildegit.org/sloum/bombadillo.git
cd bombadillo
go install
sudo make install
This should be:
If they do not run
make
beforesudo make install
then there will be no binary to install as part of the install step.@ -21,3 +21,3 @@
git clone https://tildegit.org/sloum/bombadillo.git
cd bombadillo
go install
sudo make install
running
make
beforemake install
is not required.The
install
target depends oninstall-bin
, andinstall-bin
depends onbuild
(see line 32 of the Makefile). So, thebuild
target is run whenever you runmake install
.If you prefer the idea of
make
thenmake install
I can update this.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.
@ -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`.
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 ofinstall
isclean
which removes the binary. So it will not be in the directory when the user gets to this step.Yup. It all makes sense to me now (it is morning and I am more awake, lol).
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.
Merging in. Thanks for the help understanding the make stuff (thanks to Ben too!).