WITH_SVM and build/install brotherliness #2

whilei · 2018-01-09T17:49:19Z

make install_all [WITH_SVM=1|0]
make install_geth [WITH_SVM=1|0]
make build_all [WITH_SVM=1|0]
make build_geth [WITH_SVM=1|0]

It made sense to me to always allow building or install with/without sputnik as an optional flag, with default WITH_SVM=1 (on). See what you think.

solution: use WITH_SVM var to toggle building or installing geth w/ or w/o svm. Refactor scripts to handle install/build command

tzdybal

This Makefile is used like a "reentrant bash script" ;) Not a true Makefile. There are only rules (script entry points) and no dependencies. make is powerful because it can eliminate the need of recompiling when it's not needed - that's why there are dependencies. For simple "build-once-on-ci" scenario it's not a problem, but I think that it would be very useful to make the 'Makefile' usable also for development.
The usual way of writing Makefile is to use file names as targets + some additional targets like install, clean, etc.

So in general, in my opinion:

we should have targets like: cmd/geth to build geth binary (same for every app in cmd)
by default we should build everything
install should install everything
clean should clean everything... like go clean -r ./... (maybe even with -i option?)

tzdybal · 2018-01-11T13:56:09Z

Makefile

@@ -19,8 +22,24 @@ setup: ## Install all the build and lint dependencies
 	dep ensure
 	gometalinter --install

+build_all: ## Build a local snapshot binary versions of all commands
+	./scripts/build.sh ${BINARY}


I'm not a big fan of calling custom build scripts from Makefile. The build script for SputnikVM could be turned into Makefile, and the build.sh is very simple, and could be added in this file (as few build targets or using some wildcards) .

tzdybal · 2018-01-11T14:00:54Z

Makefile

@@ -19,8 +22,24 @@ setup: ## Install all the build and lint dependencies
 	dep ensure
 	gometalinter --install

+build_all: ## Build a local snapshot binary versions of all commands
+	./scripts/build.sh ${BINARY}
+	make build_geth


To run make recursively, you should use $(MAKE) instead of plain make (see docs).

But for simple case like this, you should add dependency to target, instead of manually calling make.

tzdybal · 2018-01-11T14:10:43Z

Makefile

@@ -1,4 +1,4 @@
-.DEFAULT_GOAL := build
+.DEFAULT_GOAL := build_geth


I would build everything by default. To maintain the make && make install philosophy.

And so make cmd/geth would, as you say above, build geth in the default target build dir, eg ./bin/geth?

tzdybal · 2018-01-11T14:12:07Z

Makefile

+	$(info Building ${BINARY}/geth)
+	if [ ${WITH_SVM} == 1 ]; then ./scripts/build_sputnikvm.sh build ; else mkdir -p ./bin && go build ${LDFLAGS} -o ${BINARY}/geth ./cmd/geth ; fi
+
+install_all: ## Install all packages to $GOPATH/bin


Rename to just install. If you see Makefile, you expect to be able to run make and then make install (later probably as root).

tzdybal · 2018-01-11T14:16:24Z

Makefile

 	if [ -d ${BINARY} ] ; then rm -rf ${BINARY} ; fi

-install: ## Install to $GOPATH/src
-	go install ${LDFLAGS} ./cmd/...
-
 # Absolutely awesome: http://marmelab.com/blog/2016/02/29/auto-documented-makefile.html
 help:
 	@grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}'


This is sooooo hackish... 🥇

r8d8 · 2018-01-11T14:37:21Z

@tzdybal , checkout original PR https://github.com/ethereumproject/go-ethereum/pull/449/files
Iit was more simple in terms of command.
I made build scripts to omit makefile hussle with tabs.
Maybe we can make couple makefiles.

whilei · 2018-01-12T00:42:30Z

Great to learn about the dependency/target-checking and philosophy --

what do ya'll think of the WITH_SVM=1|0 for anytime building geth?

whilei · 2018-01-12T09:36:03Z

And @tzdybal how does one actually do such things?

make is powerful because it can eliminate the need of recompiling when it's not needed - that's why there are dependencies.

Been looking around for tuts and docs, but kind of seems like everyone on the internet learned this shit like 14 years ago

tzdybal · 2018-01-14T22:28:07Z

Because this is old school! make was introduced in... 1976!

Basically:
Targets are... target files (or wildcards matching them). The commands for each target should result in creation of that file.
And you can also have "abstract" targets (that are not files, like install, test, etc). You can think of it that way - after running all the commands there are no files with name corresponding to the target name, so you always need to re-run the target, if someone asks about it.

Dependencies are files needed to build the target (if any of those files changed on disk, recompilation is required). In the graph of dependencies, source files are leafs. Typically (when C code is compiled), there are some rules to build libraries from source files and then to link them into executables, etc.

- use 'build' as default goal - rename install_all to install ... still thinking about build script for geth/+sputnik

whilei · 2018-01-16T08:42:23Z

With this revision, is

make build
make install
make cmd/geth
make cmd/abigen
...

tzdybal · 2018-01-16T14:59:21Z

It still not what I meant ;) But actually in previous comment I was unfortunately wrong.

Instead of cmd/geth as target, I meant cmd/geth/geth (the exact name of the binary). And then IMHO should have 'in tree' build, with no extra $(BINARY) directory. So make cmd/geth/geth builds geth binary located in cmd/geth/geth. This would be the "typical way". But on the other hand, we're not going to use proper dependencies to source files anyways (because go itself does all the magic to recompile what's needed, and we don't want to specify all the .go files and dependencies in Makefile), so we're not making a "typical" Makefile. I think I need to think about this one more time after a coffee ;)

whilei · 2018-01-16T15:29:08Z

Agreed -- I haven't been able to see a solution that grabs me as definitely optimal either.

My priorities in navigating around a solution have been:

simple for devs and build-from-sourcers. This means as a few CLI steps for typical goals as reasonably possible, while keeping necessary optionalities. In this way, even favoring convenience over convention, since it's unconventional to use a makefile with a go project anyways.
plays well with go structure and "magic" (eg. re/compilation caching). installs should be congruent with go installs as far as target path ($GOPATH/bin) and... erm, build options (like -tags=sputnikvm)
at this point I'm less concerned with adding convenience for the CIs, since we're not trying to use the makefile to configure and run a particularly massive set of builds/archs/dists
i'd like to see the makefile/scripts be able to be tested against Windows, too, since I don't have hands-on with the Window nmake or ps

...

The original motivation for the makefile was to simplify the steps for building while including SputnikVM dependencies and optionality. I'd be happy to narrow the focus on this draft of the makefile away from the other cmd/*s aside from geth if it makes it easier to design the makefile command/target UI, for now at least.

tzdybal

After rethinking, I like the final solution.

whilei added 4 commits January 9, 2018 08:19

Add other dep vendor dir to excluded from fmt

ce3c5c6

Rename command 'sputnikvm' -> 'build_sputnikvm' for consistency

44b3b2c

problem: build/install inconsistent for all vs. geth

766409e

solution: use WITH_SVM var to toggle building or installing geth w/ or w/o svm. Refactor scripts to handle install/build command

Fix whitespace

e408ade

whilei mentioned this pull request Jan 9, 2018

Add build script ethereumproject/go-ethereum#449

Merged

tzdybal suggested changes Jan 11, 2018

View reviewed changes

update Makefile to use dependencies...

9ff929d

- use 'build' as default goal - rename install_all to install ... still thinking about build script for geth/+sputnik

tzdybal approved these changes Jan 17, 2018

View reviewed changes

Merge branch 'feature/build_script' into build-script

9cb59a0

r8d8 merged commit 8ff928a into r8d8:feature/build_script Feb 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WITH_SVM and build/install brotherliness #2

WITH_SVM and build/install brotherliness #2

whilei commented Jan 9, 2018

tzdybal left a comment

tzdybal Jan 11, 2018

tzdybal Jan 11, 2018

tzdybal Jan 11, 2018

tzdybal Jan 11, 2018

whilei Jan 12, 2018

tzdybal Jan 11, 2018

tzdybal Jan 11, 2018

r8d8 commented Jan 11, 2018 •

edited

Loading

whilei commented Jan 12, 2018

whilei commented Jan 12, 2018

tzdybal commented Jan 14, 2018

whilei commented Jan 16, 2018

tzdybal commented Jan 16, 2018

whilei commented Jan 16, 2018

tzdybal left a comment

		@@ -1,4 +1,4 @@
		.DEFAULT_GOAL := build
		.DEFAULT_GOAL := build_geth

WITH_SVM and build/install brotherliness #2

WITH_SVM and build/install brotherliness #2

Conversation

whilei commented Jan 9, 2018

tzdybal left a comment

Choose a reason for hiding this comment

tzdybal Jan 11, 2018

Choose a reason for hiding this comment

tzdybal Jan 11, 2018

Choose a reason for hiding this comment

tzdybal Jan 11, 2018

Choose a reason for hiding this comment

tzdybal Jan 11, 2018

Choose a reason for hiding this comment

whilei Jan 12, 2018

Choose a reason for hiding this comment

tzdybal Jan 11, 2018

Choose a reason for hiding this comment

tzdybal Jan 11, 2018

Choose a reason for hiding this comment

r8d8 commented Jan 11, 2018 • edited Loading

whilei commented Jan 12, 2018

whilei commented Jan 12, 2018

tzdybal commented Jan 14, 2018

whilei commented Jan 16, 2018

tzdybal commented Jan 16, 2018

whilei commented Jan 16, 2018

tzdybal left a comment

Choose a reason for hiding this comment

r8d8 commented Jan 11, 2018 •

edited

Loading