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

Add msi builder #19

Merged
merged 15 commits into from Sep 16, 2016
Merged

Add msi builder #19

merged 15 commits into from Sep 16, 2016

Conversation

carlpett
Copy link
Collaborator

@carlpett carlpett commented Sep 1, 2016

This adds a script which builds a (very) simple msi file. Currently it assumes that binaries for a release are uploaded to https://github.com/martinlindhe/wmi_exporter/releases/v${Version}_${Arch}.zip, since that is a common pattern, but maybe we'll want to make this part of a release pipeline? In that case we'll just need to change a few lines to pick up the binaries.

Remaining work:

  • Currently there is no way to change install dir, it will install to the appropriate Program Files directory. Is that something that needs to be possible to customize?
  • We should have a windows service option. Either we bundle winsw, or we could do something similar to bosun and integrate directly. winsw has the advantage of not adding more code to maintain, but on the other hand we take an external dependency.

@martinlindhe
Copy link
Collaborator

martinlindhe commented Sep 2, 2016

Very cool!

Perhaps the msi should be built using locally compiled wmi_exporter.exe instead? (I'm thinking how to use it in a release process - i guess most users won't run this themselves?)
Registering the app as a service would also be very nice. Regarding the installation path i think its fine to shoot it into a default path, we'll likely want this running as a unattended install as part of provisioning.

@carlpett
Copy link
Collaborator Author

carlpett commented Sep 2, 2016

Exactly, if we do this in the release, we'll just refer to the .exe we just compiled. I can make some changes so it uses a file taken as in-param.
Let's keep the install path as-is then, until it breaks someone's workflow. I've actually started rolling this out on some machines at work now, and program files is good enough for me.

So we just need to decide how the service should be managed. Own code or external dep?

@martinlindhe
Copy link
Collaborator

martinlindhe commented Sep 2, 2016

I would prefer go code to c#, in order to keep the project and dependencies in as few languages as possible. Perhaps we can refactor the bosun windows service code into a separate go package and depend upon that, unless there already is such package. I'll take a look if time permits.

@brian-brazil
Copy link
Collaborator

I think we'd want to msi file to end up as part of the release, rather than built on top of it. On the other hand I haven't really worked with windows for about a decade :)

@martinlindhe
Copy link
Collaborator

@carlpett
Copy link
Collaborator Author

carlpett commented Sep 3, 2016

@brian-brazil Good point. The script is adapted from one I wrote a while ago to create msis for projects that for one reason or another did not ship their own installers and did not want to support one.

@carlpett
Copy link
Collaborator Author

carlpett commented Sep 3, 2016

@martinlindhe Yes! I'll pull that in and also change to take a path to executable

@carlpett
Copy link
Collaborator Author

carlpett commented Sep 4, 2016

There are two approaches we can take to service installation, I've realized. Either we do what I initially suggested, add some -service.install parameter, or we let the installer manage it.
Command-line setup has the benefit that it is easy to configure the enabled collectors. On the other hand, you'll need to run a post-install script for things to work.
Letting the installer manage it makes it a single action until done, and we can start the service immediately at install-time, but the user needs to supply the collector-list to msiexec when installing.

Any preferences, or additional arguments for either side? I've already implemented both strategies to test them out, so there's no overhead from re-implementation...

The alternatives are either manual installation and startup,

msiexec /i <path-to-msi>
<path-to-install-dir>\wmi_exporter.exe -service.install -collectors.enabled foo,bar,baz -other.params
sc start wmi_exporter

(The -service.install flag just adds all remaining flags as startup-params to the service)

Or, alternatively just

msiexec /i <path-to-msi> ENABLED_COLLECTORS=foo,bar,baz

Here we also need to maintain the list of default enabled collectors in the wix file (or do some semi-advanced conditional string manipulation using xml), but the overall flow is a bit smoother.

@martinlindhe
Copy link
Collaborator

martinlindhe commented Sep 5, 2016

@martinlindhe Yes! I'll pull that in and also change to take a path to executable

that should be the exepath parameter for Install() https://github.com/martinlindhe/winservice/blob/master/winservice.go#L13
(as --exe="path to exe" in the cmd)

@carlpett
Copy link
Collaborator Author

carlpett commented Sep 5, 2016

I meant the build-script in the second part, actually, sorry for mixing two things in the same sentence :) I have got the Install() working, but as I noted I'm not entierly sure if it is the right way to go

@martinlindhe
Copy link
Collaborator

martinlindhe commented Sep 5, 2016

Ah, nevermind then :)

Regarding default enabled collectors in the wix-file,.. since wmi_collector defaults to some enabled ones if none are provided to the wmi_exporter.exe, cant we just use that? So none are provided by the wix-file, and wmi_exporter.exe then use the default?

@carlpett
Copy link
Collaborator Author

carlpett commented Sep 5, 2016

Yes, that would be best. However, currently I'm setting the service arguments to -collectors.enabled [COLLECTORS_ENABLED], which requires that there is some value there. I'll see how complicated it gets to only add this if the parameter is actually sent in.

@carlpett carlpett force-pushed the msi-package branch 3 times, most recently from 76414c7 to 783ae16 Compare September 5, 2016 20:24
@carlpett
Copy link
Collaborator Author

carlpett commented Sep 5, 2016

Ok, solved that part. The installer can now take parameters for collectors, listen addr/port and path.

Once the package is installed, a firewall exception is created, the service is installed and finally started. Great success. Now there are just a few more things...

  • Logging is lost when running as a service. I've written some code for logrus which adds an eventlog hook, but it also needs to go into prometheus common log. I'll do some cleanup and submit a patch. Maybe not a deal-breaker, but it makes troubleshooting a bit harder...
  • The appveyor setup needs some attention. It's quite hard to test from what I can tell, just push and pray? The config needs a token and probably a bunch of fixes around artifact upload that I haven't been able to test.
  • Some docs on how to set parameters for the installer

I had to make some changes to the listener in order for the service to be able to shut it down (replace ListenAndServe with a separate Listener and a stop-channel). Would be great with some scrutiny there to see that I haven't missed anything.

Also, it turns out that the winservice stuff you pulled from scollector was meant to controlling/install services, not being run as one. So I've dropped that dependency, and implemented the necessary interface for interactive with the service system directly in exporter.go. It might be possible to factor that out.

@carlpett
Copy link
Collaborator Author

carlpett commented Sep 5, 2016

(I'm not really fond of how vendoring makes PRs really blow up, eg +18,647 −2. Also makes GitHub's online diff viewer crash on my machine...)

@martinlindhe
Copy link
Collaborator

martinlindhe commented Sep 6, 2016

The appveyor setup needs some attention. It's quite hard to test from what I can tell, just push and pray? The config needs a token and probably a bunch of fixes around artifact upload that I haven't been able to test.

appveyor runs on all PR's, and should pass assuming govendor build . passes

EDIT: gotcha! didnt see you extended appveyor to build the msi in this pr

@martinlindhe
Copy link
Collaborator

martinlindhe commented Sep 6, 2016

Also, it turns out that the winservice stuff you pulled from scollector was meant to controlling/install services, not being run as one. So I've dropped that dependency, and implemented the necessary interface for interactive with the service system directly in exporter.go. It might be possible to factor that out.

Yes, I thought that was what we needed? :) Glad you sorted it out then

@carlpett
Copy link
Collaborator Author

carlpett commented Sep 9, 2016

I've created a PR to common/log, prometheus/common#56. As soon as that is merged, I can push my updates (because at the moment govendor will blow up). At that point, the major remaining thing is to fix the appveyor setup.
@martinlindhe Could you create an encrypted access token in the AppVeyor account, so that it can upload artifacts? See https://www.appveyor.com/docs/deployment/github/.

martinlindhe added a commit that referenced this pull request Sep 9, 2016
@martinlindhe
Copy link
Collaborator

@carlpett I've added a deploy section to appveyor.yml. Let's find out if it works.

FYI, I omitted one tag that might be relevant for it all to function, this one:
artifact: /.*\.nupkg/ # upload all NuGet packages to release assets

martinlindhe added a commit that referenced this pull request Sep 9, 2016
@carlpett
Copy link
Collaborator Author

Alright, the PR is merged in common, so I've updated the reference, updated the AppVeyor script with the token, and added some dependencies that I missed (is there a way to build with govendor locally without using all the packages that I have lying around?).
So, if anyone has a bit of time to poke the changes I've made before merging, that'd be great :)

I think that we can't know if the AppVeyor stuff actually works until we've merged this and created a tag, sadly. But it should be fairly quick to fix if it doesn't, one the rest is in master.

@martinlindhe
Copy link
Collaborator

martinlindhe commented Sep 12, 2016

(is there a way to build with govendor locally without using all the packages that I have lying around?).

to build wmi_exporter.exe using govendor, the following seems to work

govendor build -v +program .

(EDIT: corrected instructions)

@martinlindhe
Copy link
Collaborator

martinlindhe commented Sep 12, 2016

Testing out the branch

  • compiled wmi_exporter.exe
  • built a msi using .\installer\build.ps1 -PathToExecutable .\wmi_exporter.exe -Version 0.1.0 -Arch "amd64"
  • installed msi using msiexec /i C:\Users\vm\dev\go\src\github.com\martinlindhe\wmi_exporter\installer\Output\wmi_exporter-0.1.0-amd64.msi ENABLED_COLLECTORS=os LISTEN_PORT=5000 (full path needed or else msiexec gave me error)
  • i see the service is registered and "started" in Services. Path to executable is "C:\Program Files\wmi_exporter\wmi_exporter.exe" -log.format logger:eventlog?name=wmi_exporter -collectors.enabled os -telemetry.addr :5000

However, localhost:5000 does not load. Also tested with port 9182, same issue. Firewall related?

Also, uninstalling the msi with msiexec /x path-to.msi leaves the service installed and running

@martinlindhe
Copy link
Collaborator

martinlindhe commented Sep 12, 2016

Confirmed with adbfb7d the service now works 😄

Sorry I did not notice that code change in the original commits, the vendor-folder is really polluting the diffs, making it hard to review. Google came up with this "https://stackoverflow.com/questions/20120478/ignoring-specific-files-file-types-or-folders-in-a-pull-request-diff" (TLDR: cannot easily be done).

I guess only work around to avoid huge patches later on would be to split such PR in two, first one to update vendor folder with new dependencies, and a second PR making use of those dependencies.

@carlpett
Copy link
Collaborator Author

carlpett commented Sep 12, 2016

However, localhost:5000 does not load. Also tested with port 9182, same issue. Firewall related?

No, bug. During the work to make it work as a service, it looks like I managed to forget to attach the http listener... Really wierd, because I tested a lot. Apparently I must have been using an old msi or something from before that change (also quite disturbing).
Fixed that.

Also, uninstalling the msi with msiexec /x path-to.msi leaves the service installed and running

This I cannot reproduce? Are you looking in the Services console? Did you refresh after uninstalling?

@carlpett
Copy link
Collaborator Author

Re the horrible diffs, I actually contacted GitHub about it yesterday. They make no promises, but "I have definitely added it to the feature request list and will share it with the team for consideration.". So, maybe some day, things will be better...

Since AppVeyor builds the PRs (and Github hides the merge button if the check fails, right?) the PR has to contain all the changes necessary to build. I guess we could keep the vendor changes separate, somehow, but it won't be smooth. On the other hand, not being able to review the interesting changes is worse.

@carlpett
Copy link
Collaborator Author

There is an irritating issue remaining, as well - since we do not have a "MessageFile", all eventlog messages are prefixed with a few paragraphs about not knowing how to format the message. The message itself turns up at the end, so not a deal breaker, but pretty irritating.

Three options:

  1. Ignore it
  2. Create a message file. This is a bit bothersome, sadly, and requires a few winsdk tools to compile.
  3. Piggyback on some existing message file. This will create an implicit dependency though, so not very nice (eg to the .net framework)

@carlpett
Copy link
Collaborator Author

I figured out a way to test the build-stuff, so now it seems to work with artifact uploading as well. Should we postpone the message file for now @martinlindhe?

@martinlindhe
Copy link
Collaborator

@carlpett Yes, i think it looks good to merge. I created a separate issue about the MessageFile in #22

@carlpett carlpett merged commit b3567c3 into prometheus-community:master Sep 16, 2016
@martinlindhe
Copy link
Collaborator

🎉

@carlpett carlpett changed the title WIP: Add msi builder Add msi builder Sep 17, 2016
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.

None yet

3 participants