Conversation
c4ffbe0 to
1297ad1
Compare
ianlivingstone
left a comment
There was a problem hiding this comment.
Overall this looks good, the thing we need to think about is how this fits into the current build/package/upload process we take during a release.
Today, it's all done locally through the Makefile via the release-all target.
In future, I think this will probably need to be done separetely, on *nix for releasing the linux/macosx and then on a separate machine for windows.
Ideally, we can figure out how to do the releasing component securely via appveyor then this can just be done on build of a release tag by appveyor.
For now, I've left some comments throughout the PR to kick off a few discussions around how to get this into mergable shape. I think our best bet is to create a new target in the Makefile for bootstrapping locally (e.g. get go-msi and wix installed) on windows and another for actually building and packaging up the msi.
Thoughts?
appveyor.yml
Outdated
| install: | ||
| # wix setup | ||
| - set PATH=%WIX%\bin;%PATH% | ||
| # go-msi install - static link method |
There was a problem hiding this comment.
I wonder if these should be in a bootstrap-windows component of the Makefile?
There was a problem hiding this comment.
Agreed, I'll add mingw32-make to appveyor so we're able to use the Makefile in appveyor.yml.
appveyor.yml
Outdated
|
|
||
| build_script: | ||
| - c:\gopath\bin\dep.exe ensure | ||
| - go build -o torus.exe |
There was a problem hiding this comment.
Hmm, we're probably going to have to separate out building the windows binary (and now the installer + uploading it) from the rest of the Makefile when we build a release through make release-all.
If you want to introduce a make msi that requires the os to be windows and for wix/go-msi to be available (if possible) then I think we can open another issue (or subsequent PR) that has the changes to actually separately build, package, and upload a windows release.
There was a problem hiding this comment.
I introduced an initial make msi that checks for go-msi, but I'll add os detection and hook it into the appveyor.yml.
packaging/msi/templates/product.wxs
Outdated
| <Component Id="ApplicationFiles" Guid="{{.Files.GUID}}"> | ||
| {{range $i, $e := .Files.Items}} | ||
| <File Id="ApplicationFile{{$i}}" Source="{{$e}}"/> | ||
| <!-- |
There was a problem hiding this comment.
We can remove this right? Since we're not installing it as a service?
|
|
||
| <Media Id="1" Cabinet="product.cab" EmbedCab="yes"/> | ||
|
|
||
| <Upgrade Id="{{.UpgradeCode}}"> |
There was a problem hiding this comment.
This is just a rule for forcing the upgrade of Torus right? Is there a way we can run a command at the same time?
When we upgrade an existing install we should also restart the daemon via torus daemon stop. Ideally we also tell them that after upgrading they will need to log into Torus again via the command prompt (or whichever shell they use).
wix.json
Outdated
| "product": "torus", | ||
| "company": "manifoldco", | ||
| "license": "LICENSE.md", | ||
| "version": "0.0.1", |
There was a problem hiding this comment.
This is the version of the binary right? Ideally this file can be tempalted like package.json inside packaging/npm which is then filled in prior to packaging.
This way we can pull the correct version in as well.
wix.json
Outdated
| "company": "manifoldco", | ||
| "license": "LICENSE.md", | ||
| "version": "0.0.1", | ||
| "upgrade-code": "8615055C-D8E0-404C-93BE-441C503BA6F0", |
There was a problem hiding this comment.
What sets this upgrade code?
There was a problem hiding this comment.
$ go-msi set-guid generates them
| @@ -0,0 +1,9 @@ | |||
| From: {{.Choco.LicenseURL}} | |||
There was a problem hiding this comment.
Thinking a bit about this, for now it's probably easiest just to get the wix support into the build pipeline so we're producing and uploading a real msi installer, then come back to thinking about Choco support.
Thoughts?
There was a problem hiding this comment.
Sounds good, I'll remove the Chocolatey artifacts for now.
added msi target to makefile
- msi make target - bootstrap-windows make target - removed chocolatey packaging - updated appveyor.yml to use makefile uncommented upgrade code
added msi target to makefile
- msi make target - bootstrap-windows make target - removed chocolatey packaging - updated appveyor.yml to use makefile
1297ad1 to
11d49f0
Compare
| @@ -0,0 +1,38 @@ | |||
| <?xml version="1.0"?> | |||
There was a problem hiding this comment.
We don't need these templates for choco right?
Makefile
Outdated
| # Generate Windows .msi | ||
| ################################################# | ||
|
|
||
| msi: binary |
There was a problem hiding this comment.
I think this should depend on the release-binary-windows rule? This way it also gets put inside the builds/ directory. The subsequent msi should probably also be placed there as well, so it gets deleted on make clean.
ianlivingstone
left a comment
There was a problem hiding this comment.
Just two changes:
- See if we can depend on
binary-windowsfor building the binaries and having them populated into thebuilds/folder. - The templates for choco should probably be removed as nothing will be using them right now.
Otherwise this looks great! Once we've resolved those two points and we've rebased this onto master, this is ready for merge :)
- msi target now relies on release-binary - msi target outputs to builds folder - wix.json relies on torus.exe existing in builds folder
|
❌ Build torus-cli 400 failed (commit d7c612ed5e by @morochena) |
|
✅ Build torus-cli 409 completed (commit 64ae3225fa by @morochena) |
wix.json
Outdated
| @@ -0,0 +1,30 @@ | |||
| { | |||
There was a problem hiding this comment.
This shouldn't be checked in right as it's a templated file?
Uses go-msi to generate a .msi installer for windows, and allows for distribution via chocolately.