Smarter glide install procedure #313

stu-gott · 2017-07-13T19:48:28Z

Also fixes an error: glide install was not stripping vendor folder.

Signed-off-by: Stu Gott sgott@redhat.com

rmohr

What I miss, is that when I now run make sync explicitly, that it will not check if my vendor folder really has the right content. I tried deleting something in the vendor folder, and then I could not re-sync.

rmohr · 2017-07-14T08:42:27Z

Makefile

@@ -32,8 +32,8 @@ distclean: clean
 	find vendor/ -maxdepth 1 -mindepth 1 -not -name vendor.json -exec rm {} -rf \;
 	rm -f manifest/*.yaml

-sync:
-	glide install
+sync: .glide.status


This file needs to be deleted on make distclean then.

berrange · 2017-07-14T08:49:57Z

Skipping checking existence of every single file in the vendor/ directory is the very reason for this change - that checking significantly slows down build cycles to check for a problem that is of the developer's own making. If you want to purge vendor code, don't delete individual files in the vendor dir, just blow away the whole tree and let it be recreated from scratch.

rmohr · 2017-07-14T08:54:38Z

I understand that. It is just counter intuitive, that when I run make sync, that it does not detect that. There are solutions like a "cheap" internal sync, which is normally used. For as long as a direct call to make sync will detect invalid vendor folders, and as long as make distclean does it's job, I am happy.

rmohr · 2017-07-14T08:57:41Z

~~@stu-gott also keep in mind, that we cache the vendor dependencis in travis. Make sure to add the status file to the cache.~~ Edit: Not necessary

stu-gott · 2017-07-17T12:53:45Z

In fairness, "glide install --strip-vendor" only adds 5 second clock time to a build (assuming everything is cached). Characterizing that as a significant delay might be overstating the problem a bit.

@rmohr is right--we could do better. I added a lazy sanity check--if the vendor folder is newer than .glide.status a full sync needs to occur. This won't cover cases where something deeply nested in the vendor folder has changed, but changes directly inside the vendor folder will be noticed.

berrange · 2017-07-17T13:10:58Z

@stu-gott on my machine running "glide install --strip-vendor" adds 15 seconds - the actual compile of go code can be as little as 5 seconds, depending on scope of what has changed since previous compile. That is a significant delay.

rmohr · 2017-07-17T14:51:17Z

Makefile

+	test -d vendor && \
+	test -f .glide.status && \
+	test '!' .glide.status -ot vendor || \
+		glide install --strip-vendor && touch .glide.status


Ok, that goes into a way too complex direction for me. Can't we do something simple, like do a .glide.status check when I run "make build", and do an explicit glide install --strip-vendor, when I run make sync?

We currently call "make sync" twice inside our build process.

EDIT: that is to say--when ./cluster/sync.sh is called

Yes, so why not doing something like:

build: .glide.status fmt vet compile .glide.status: glide.yaml glide update --strip-vendor && touch $@ sync: glide install --strip-vendor

Then you have a lightweight check when you build code, which should give you a reasonable good dependency update experience, and a make sync target, which allows you to be sure that everything is up to date.

rmohr · 2017-07-17T14:52:13Z

Makefile

@@ -59,4 +63,9 @@ vagrant-sync-build: build
 vagrant-deploy: vagrant-sync-config vagrant-sync-build
 	export KUBECTL="cluster/kubectl.sh --core" && ./cluster/deploy.sh

+.glide.status: glide.yaml
+	if test -f $@; then \
+		glide update --strip-vendor && touch $@; \


Why is it her glide update and glide install on the sync target?

This way glide update only gets triggered if glide.yaml changed. The check for glide install needs to be done every time sync activated. Part of me wants to take this entire section out--and make glide update be something that's only ever done intentionally (and manually).

stu-gott · 2017-07-17T16:31:14Z

Since we don't have a unified consensus on how this patch needs to work, I'm going to split the fix for using "glide install --strip-vendor" into a separate PR.

stu-gott · 2017-07-17T16:43:03Z

Please note that this PR is now rebased onto: #323

rmohr · 2017-07-17T16:49:08Z

Makefile

@@ -59,4 +63,9 @@ vagrant-sync-build: build
 vagrant-deploy: vagrant-sync-config vagrant-sync-build
 	export KUBECTL="cluster/kubectl.sh --core" && ./cluster/deploy.sh

+.glide.status: glide.yaml
+	if test -f $@; then \


Why are you testing for the existence of the file?

Upon first switching to this approach (or after using distclean), all developer's systems will have a glide.yaml but not a .glide.status. It makes more sense to assume an "install" is needed in that case--as opposed to running update--which would get everybody's glide.lock out of sync

rmohr · 2017-07-17T16:52:16Z

Makefile

+	test -d vendor && \
+	test -f .glide.status && \
+	test '!' .glide.status -ot vendor || \
+		glide install --strip-vendor && touch .glide.status


Yes, so why not doing something like:

build: .glide.status fmt vet compile .glide.status: glide.yaml glide update --strip-vendor && touch $@ sync: glide install --strip-vendor

Then you have a lightweight check when you build code, which should give you a reasonable good dependency update experience, and a make sync target, which allows you to be sure that everything is up to date.

rmohr

So I think, if we want to keep the vendor fetching as part of the build process (which I like, but what we don't have to), then we have to make that a little bit smarter:

First some clarifications:

glide update --skip-vendor works with the glide.yaml file. It download all dependencies there, goes recursively through the nested dependencies, flattens them, installs them and creates a glide.lock file.
glide install --skip-vendor works with the glide.lock file. It just goes through the list and installs the mentioned dependencies in that list in the vendor folder.
Both glide.yaml and glide.lock are checked in to git when they are changed. Therefore, for as long as you don't change dependencies, you only have to run glide install --skip-vendor, and not a full glide update --skip-vendor when you update your git repository.

Then some possible workflows:

When I change a dependency, I run make sync, to update my glide.lock file, and then I can commit everything.

Here glide update would always be run.

When I rebase/checkout the GIT repo, I want to be able to just run make, without having to care about dependencies.

To allow the second flow, make like rules which work on timestamps are not good enough. However, we could introduce a .glide.lock.md5 and a .glide.yaml.md5 file. Then, in the corresponding force-make targets, we check if the md5 sum has changed, and only then touch/update .glide.lock.md5 with the new md5 sum, which would then cause a glide install or glide update invocation.

This also plays nice with the make sync invocation, because the md5 sum in the status files only changes, if really one of the files has changed.

rmohr · 2017-07-18T07:26:51Z

Makefile

+	rm -f .glide.status
+
+checksync: .glide.status
+	test -d vendor || make sync

 sync:
 	glide install --strip-vendor


Sorry for asking so many questions, but is not glide install the one which takes only glide.lock into account (with other words the cheaper operation?)? AFAIK glide update is more expensive, since it goes through glide.yaml, checks out all dependencies, goes through all subdependencies there, prepares the vendor folder, and finally updates the glide.lock file.

This is correct. glide install is cheaper and uses a cache. glide update crawls all deps and refreshes glide.lock.

rmohr · 2017-07-18T07:39:06Z

Makefile

+	rm -f .glide.status
+
+checksync: .glide.status
+	test -d vendor || make sync


glide does not need the vendor folder, why do you check for it? It will create it if it is not there.

For the current implementation if glide.yaml is updated, glide update gets called right before this check. Without this check glide install will blow away and re-install a freshly laid vendor folder.

rmohr · 2017-07-18T07:40:24Z

Makefile

-.PHONY: build fmt test clean distclean sync docker manifests vet publish vagrant-sync-config vagrant-sync-build vagrant-deploy functest
+.glide.status: glide.yaml
+	if test -f $@; then \
+		glide update --strip-vendor; \


I think glide install is correct here.

stu-gott · 2017-07-18T12:19:11Z

We're pretty much in perfect agreement with regards to your clarifications above. The only difference between what you're envisioning and what's in place currently is what make sync is supposed to do. right now:

make sync just fixes your vendor folder using glide install. This is the exact behavior from a few weeks ago when we were using govendor.
If you update your glide.yaml file and then run make, the Makefile rules will catch that and execute glide update.

However, you make an extremely good point about checking out different branches with possibly different glide.lock files--this Makefile ignores that case. Keep in mind that fixing this issue using .md5 files has the potential to get complicated/over-engineered.

~~Personally I don't mind the current implementation in master--where glide install is run every time. It's less complicated and works every time--at the cost of a few seconds of build time.~~

EDIT: It turns out, I do mind if glide install is run every time. When executing make test, replacing the vendor folder triggers a complete re-compile of the entire codebase--which does increase the runtime of the test suite by orders of magnitude.

Signed-off-by: Stu Gott <sgott@redhat.com>

stu-gott · 2017-07-20T15:03:27Z

New approach:

If .glide.yaml.lock does not exist, assume this is a fresh install (so just run make sync)
If glide.yaml changes (as verified by MD5), run glide update
If glide.lock changes (e.g. switched branches), run glide install

rmohr

Maybe make sync can be changed, but good enough for now.

rmohr · 2017-07-21T07:50:25Z

Makefile

+		${HASH} glide.yaml > .glide.yaml.hash; \
+		${HASH} glide.lock > .glide.lock.hash; \
+	elif [ "`${HASH} glide.lock`" != "`cat .glide.lock.hash`" ]; then \
+		make sync; \


glid update seems to be right here

rmohr · 2017-07-21T07:50:44Z

Makefile

+		${HASH} glide.lock > .glide.lock.hash; \
+	elif [ "`${HASH} glide.lock`" != "`cat .glide.lock.hash`" ]; then \
+		make sync; \
+	fi

 sync:


I would have expected a glide update here

* Do file conversion using go pkgs instead of cli tools Refactored copyIfNotPresent for clarity doTar uses archive/tar instead of cli * Convert xz and gzip to use go pkgs Remove vendor from .gitignore * Exchange github.com/xi2/xz with github.com/ulikunitz/xz dependency for writer functionality Fixed xz import path * Review comments

fabiand requested review from berrange and rmohr July 14, 2017 08:39

rmohr requested changes Jul 14, 2017

View reviewed changes

rmohr reviewed Jul 17, 2017

View reviewed changes

stu-gott force-pushed the fix-glide-install branch from aa6de2d to 19b61f5 Compare July 17, 2017 16:42

rmohr requested changes Jul 17, 2017

View reviewed changes

stu-gott force-pushed the fix-glide-install branch from 19b61f5 to 3a186b4 Compare July 17, 2017 22:01

rmohr reviewed Jul 18, 2017

View reviewed changes

Smarter glide install procedure

528849e

Signed-off-by: Stu Gott <sgott@redhat.com>

stu-gott force-pushed the fix-glide-install branch from 3a186b4 to 528849e Compare July 20, 2017 15:00

rmohr approved these changes Jul 21, 2017

View reviewed changes

rmohr merged commit 4664c00 into kubevirt:master Jul 21, 2017

stu-gott deleted the fix-glide-install branch May 17, 2018 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Smarter glide install procedure #313

Smarter glide install procedure #313

stu-gott commented Jul 13, 2017

rmohr left a comment

rmohr Jul 14, 2017

berrange commented Jul 14, 2017

rmohr commented Jul 14, 2017

rmohr commented Jul 14, 2017 •

edited

Loading

stu-gott commented Jul 17, 2017 •

edited

Loading

berrange commented Jul 17, 2017

rmohr Jul 17, 2017

stu-gott Jul 17, 2017 •

edited

Loading

rmohr Jul 17, 2017

rmohr Jul 17, 2017

stu-gott Jul 17, 2017

stu-gott commented Jul 17, 2017

stu-gott commented Jul 17, 2017

rmohr Jul 17, 2017

stu-gott Jul 17, 2017

rmohr Jul 17, 2017

rmohr left a comment

rmohr Jul 18, 2017

stu-gott Jul 18, 2017

rmohr Jul 18, 2017

stu-gott Jul 18, 2017 •

edited

Loading

rmohr Jul 18, 2017

stu-gott commented Jul 18, 2017 •

edited

Loading

stu-gott commented Jul 20, 2017

rmohr left a comment

rmohr Jul 21, 2017

rmohr Jul 21, 2017

Smarter glide install procedure #313

Smarter glide install procedure #313

Conversation

stu-gott commented Jul 13, 2017

rmohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berrange commented Jul 14, 2017

rmohr commented Jul 14, 2017

rmohr commented Jul 14, 2017 • edited Loading

stu-gott commented Jul 17, 2017 • edited Loading

berrange commented Jul 17, 2017

Choose a reason for hiding this comment

stu-gott Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stu-gott commented Jul 17, 2017

stu-gott commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stu-gott Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stu-gott commented Jul 18, 2017 • edited Loading

stu-gott commented Jul 20, 2017

rmohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmohr commented Jul 14, 2017 •

edited

Loading

stu-gott commented Jul 17, 2017 •

edited

Loading

stu-gott Jul 17, 2017 •

edited

Loading

stu-gott Jul 18, 2017 •

edited

Loading

stu-gott commented Jul 18, 2017 •

edited

Loading