Add assets loading, binary selection and VCS information in manifest #13
Conversation
Just a tidbit of refactoring.
Add --use-binary option which allows user to choose which binary should be placed in the image if go get builds multiple binaries. This change also introduces some changes in naming - when project example.com/foo/bar has binaries bar and quux, then for quux the created image will have a filename bar-quux.aci, name in manifest will be example.com/foo/bar-quux.
Add --asset option which takes two paths separated with OS specific path separator (colon on Unices, semicolon on Windows). The first path specifies a path inside produced image, second part - the path from local filesystem which will be copied into image. We handle only regular files, directories and symlinks that does not point outside the asset.
This is strictly developer option to be able to view the contents of /tmp/goaciXXX directory, when something goes wrong.
It supports getting information from git, hg, svn and bzr.
The goaci.go already looks like a kitchen sink with two year old leftovers. Let's clean it up a bit.
If a symlink target is an absolute path still pointing somewhere inside the asset it sits in then preserve the absoluteness even in rootfs. That requires a bit of path munging.
Add two placeholders - GOPATH and PROJPATH. GOPATH placeholder is obvious. PROJPATH placeholder will be replaced with absolute path to project go get is building. Usage is simple: --asset /assets:<PROJPATH>/assets
Split out some code to separate functions with hopefully self-explanatory names. The additional aim is to have function fit into single screen.
Remove some more clutter from goaci.go.
We are using flag package for option parsing, so stop using os.Args. We can take the project from flag.Args after ensuring that it has one and only one string.
This basically rewrites goaci.go into many smaller and clearer functions. It also removes all uses of die function, so the deferred removal of /tmp/goaciXXX is actually executed (die used os.Exit() which prevented that).
|
@iaguis : you said out of band that you had a problem with using goaci on etcd with this branch? Is it still the case? |
| // goaci.aci. For project with multiple binaries | ||
| // (github.com/appc/spec/...) returned values would be (assuming ace | ||
| // as selected binary): github.com/appc/spec, github.com/appc/spec-ace | ||
| // and spec-ace.aci. |
There was a problem hiding this comment.
I am not convinced by the need to change the ACName and the ACI filename. I don't think the user would want to create several ACIs with different names and each pointing to a different binary. Rather, they would produce one ACI containing all the binaries and starting the main binary selected by --use-binary.
The current etcd ACI (built without goaci) contains etcd, etcdctl and documentation files. The documentation can be added with --asset but including several binaries in a single ACI is useful in general: the main binary could fork and execute the other binaries. (I don't think etcd executes etcdctl but other projects could do that)
I see that etcdctl is not build by go build. It is only built by ./build. Does it means there is no way to include both etcd and etcdctl in the same ACI? I wonder if there is a way to reorganise the etcd sources so that go build would produce both binaries and to include them both in the ACI.
There was a problem hiding this comment.
My impression is that ACI image is basically a package of one standalone application and its assets, in this case - etcd daemon. If you want more binaries, create more ACI images. That's why I'm appending binary name to project name if it produces more than one binary. The case would be even worse if you have project foo that builds two daemons bar and baz. I suppose it would be better to create differently named images for daemon bar (like foo-bar.aci) and for daemon baz (like foo-baz.aci) instead of having foo.aci and wondering if it runs bar or baz daemon.
If a binary forks and executes other binaries then it is builder's responsibility to add them as an asset.
As of ./build - sounds like user has to run goaci github.com/coreos/etcd/... to get both etcd and etcdctl binary. I haven't tested that.
| return fmt.Errorf("Can't handle local asset %v - not a file, not a dir", fi.Name()) | ||
| } | ||
|
|
||
| func PrepareAssets(assets []string, rootfs string, paths map[string]string) error { |
There was a problem hiding this comment.
No need to export PrepareAssets with a capital P since it is not used outside of package main.
There was a problem hiding this comment.
Same case in Warn function case - I prefer to leave for future librarification.
|
Great review, @alban. I looked into it as well and I couldn't find anything else. |
|
Thanks for review @alban, the fixes will come with followup commits. |
That simplifies the code a lot for a short while. Later some of it will probably come back when we implement checking for dangling symlinks after assets are copied.
These usually are filenames, so it's good to delimit them with quotes in case there are spaces in them.
| return v, nil | ||
| } | ||
| } | ||
| return "", fmt.Errorf("No such binary found in gobin: %q. There are following binaries available: \"%s\"", opts.useBinary, strings.Join(names, `", "`)) |
There was a problem hiding this comment.
This line has both %q and %s. Is it intentional?
There was a problem hiding this comment.
Urgh, one second. Fix coming.
|
Except the couple of nitpicks, all the new commits look good to me. |
Ditto. |
This PR fixes #4, #6 and #7. It also refactors a code which might look like a rewrite. Please see the issues mentioned to see how fixes were implemented.