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

GNUmakefile: add install target for linux systems #4365

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

leongross
Copy link
Contributor

Small PR adding the option to quickly install custom build binaries to the system path, similar to the way the docs describe it. There are 2 options: local and system-wide installation. The default is system-wide installation.

# defaults to system installation
make install

# system installation, INSTALL_SYSTEM_DIR defaults to /usr/bin
INSTALL_SYSTEM_DIR=<customDir> make install

# user installation, INSTALL_LOCAL_DIR defaults to $HOME/go/bin
INSTALL_LOCAL=1 INSTALL_LOCAL_DIR=<customDir> make install

@leongross leongross marked this pull request as ready for review July 26, 2024 12:46
@aykevl
Copy link
Member

aykevl commented Aug 2, 2024

Is there a reason you want to put it in a system path, and not effectively run go install (that puts it in $GOBIN)? Putting it in $GOBIN would make much more sense to me (and in fact, I'd use that a lot if it were available).

@leongross
Copy link
Contributor Author

On a multi-user system, it might be useful to install the binary on a system path.
Especially when uninstalling the tinygo package from the system path via the systems package manager for development purposes.
But the local installation should be replaced by a go install. I still think it is a good idea to wrap this into a make install, since all the rest of the build system is also wrapped into that so it would just make sense.

@leongross leongross changed the title build: add install target for linux systems GNUmakefile: add install target for linux systems Aug 12, 2024
@leongross
Copy link
Contributor Author

@dkegel-fastly I saw you have been working on the GNUmakefile, thoughts on this PR?

@@ -36,6 +38,10 @@ GOTESTPKGS ?= ./builder ./cgo ./compileopts ./compiler ./interp ./transform .
# tinygo binary for tests
TINYGO ?= $(call detect,tinygo,tinygo $(CURDIR)/build/tinygo)

# isntalltion location defaults
INSTALL_LOCAL_DIR ?= $(HOME)/go/bin/
Copy link
Contributor

@dkegel-fastly dkegel-fastly Aug 12, 2024

Choose a reason for hiding this comment

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

Have you considered providing the old-school standard variables and targets that packagers rely on and experienced "make" users expect?

In particular, I think autoconf-generated configure scripts let users control installation location in two ways

make install prefix=/alternate/directory
make install DESTDIR=/alternate/directory

If I understand correctly, both are needed because the first method works on windows (as it replaces the whole directory including drive letter), and the second is expected by e.g. dh_auto_install, but they might have identical effect for us.

Debian packagers would thank you :-)

See https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.72/html_node/Installation-Names.html
and e.g. https://manpages.debian.org/testing/debhelper/dh_auto_install.1.en.html

Copy link
Contributor

@dankegel dankegel Aug 12, 2024

Choose a reason for hiding this comment

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

Gosh darn it, now I remember: GNUmakefile is only for the static build. "Make install" should copy build/TinyGo, not do "go install". Glad you got that right.

Also, using profile and DESTDIR must stack; DESTDIR applies after profile. (It is for package wranglers only.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dkegel-fastly for the feedback. I should really adapt this patch to the convention utilizing prefix and DESTDIR.

Copy link
Contributor

Choose a reason for hiding this comment

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

also fix the typos in the comments else the dreaded Spelling Police will come after you :-)

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.

4 participants