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

Implemented internal plugins #2608

Closed
wants to merge 16 commits into from
Closed

Implemented internal plugins #2608

wants to merge 16 commits into from

Conversation

cbednarski
Copy link
Contributor

  • Internal plugins are compiled into the same packer binary and invoked through the plugin command
  • Search paths allow disk-based plugins to override and should function as normal
  • This should allow for a 94% disk space savings vs statically compiling all the plugins as separate binaries -- approximately 24mb vs 431mb
  • This should also make go get github.com/mitchellh/packer work out of the box, without the need for make
  • Compiles faster since there's only one binary
  • etc.

The build scripts are not updated to use this and there is a name collision with the packer folder, so for the time being you can build this with:

go build -o packer_ .
mv packer_ /usr/local/bin/packer

- Internal plugins are compiled into the same packer binary and invoked through the plugin command
- Search paths allow disk-based plugins to override and should function as normal
- This should allow for a 94% space savings vs statically compiling all the plugins as separate binaries.. approximately 24mb vs 431mb
@dch
Copy link

dch commented Aug 17, 2015

Thanks @cbednarski this is a huge saving for smaller laptops lots of which sit around the 128GB mark. Hope to test this tomorrow. Build works fine, I needed to

go get github.com/mitchellh/packer
cd $GOHOME/src/github.com/mitchellh/packer
curl https://patch-diff.githubusercontent.com/raw/mitchellh/packer/pull/2608.patch | patch -p1
go get -u -v
go build -o /usr/local/bin/packer  .

@vtolstov
Copy link
Contributor

Does it possible to add tags to not build some commands?
For example novmware,nodocker and so?
In this case I can speedup build and strip binary if I know that I don't need vmware or openstack stuff.


server, err := plugin.Server()
if err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we c.UI.Error and exit 1 here instead of panicicing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add some more friendly error messaging but users can't call the plugin on their own. Currently if a user tries to run a plugin packer crashes and says "don't do that". This essentially preserves the existing behavior.

@cbednarski
Copy link
Contributor Author

@vtolstov With the one-file-per-plugin approach it is feasible to use a flag to build selectively. With this new approach I think it is actually more complicated. Since you only need to build one file for packer which includes all the plugins, it's actually very fast and small even with all the plugins, so I don't think it's very important to provide a flag now.

Also, custom plugins should still work the same as before. I still need to update the build and release tooling but for now you can use go build to play around with this.

@vtolstov
Copy link
Contributor

@cbednarski in case of using artifice post processor i get error:

1439897935,,ui,error,rpc: can't find service PostProcessor.Configure

@vtolstov
Copy link
Contributor

@cbednarski please check pluginType in case of post (i'm write info below)

@cbednarski
Copy link
Contributor Author

@vtolstov I switched the implementation to use a regexp instead of string split. This implementation should be correct and also less code / easier to maintain. I tested with artifice and it works. Let me know if you still have any problems with it!

@vtolstov
Copy link
Contributor

@cbednarski may be instead of using if/else if use switch/case statement?

@cbednarski
Copy link
Contributor Author

@vtolstov Great feedback! Python habits. ;)

@vtolstov
Copy link
Contributor

LGTM for me, using all the day and not see any issues with this pr.
So +1 for merge =)

@mitchellh
Copy link
Contributor

Only thing: take a look at this to "hide" certain commands from the help: https://github.com/mitchellh/cli/blob/8102d0ed5ea2709ade1243798785888175f6e415/help.go#L63

We use it in Vault I think. But I think we should do that here to hide it from packer output.

@cbednarski
Copy link
Contributor Author

Only thing: take a look at this to "hide" certain commands from the help:

@mitchellh Nice! I was trying to figure out how to do that.

@cbednarski cbednarski added this to the v0.9.0 milestone Aug 21, 2015
@cbednarski cbednarski deleted the f-integrated-plugins branch October 22, 2015 00:06
@cbednarski
Copy link
Contributor Author

This was rebased and merged via #2854

@orivej
Copy link
Contributor

orivej commented Dec 1, 2015

It made input handling by ui.Ask() unreliable: sometimes a line of input is captured, sometimes it is dropped (ui.Ask() does not return after pressing Enter). This is easily reproducible when running packer -debug.

@orivej
Copy link
Contributor

orivej commented Dec 3, 2015

Input handling in built-in plugins breaks because they call setupStdin() in wrappedMain(). AFIAU, plugins inherit terminal stdin, even though they are not supposed to read it, but setupStdin() spawns a stdin reader which races with main process for input.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants