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.

Sign up for GitHub

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

Jump to bottom

Use wix toolset to package windows installer #28

Merged
merged 13 commits into from
May 28, 2017
Merged

Use wix toolset to package windows installer #28

merged 13 commits into from
May 28, 2017

Conversation

syxolk
Copy link
Collaborator

@syxolk syxolk commented May 28, 2017

What is needed:

  • Python 3 (Python 2 may work as well)
  • Python toml package
  • pandoc
  • Call make_installer.ps1 from within the packaging/wix directory

What it does:

  • Inserts the current version from Cargo.toml into indentex.wxs
  • Generates LICENSE.rtf from LICENSE.md
  • Generates indentex.msi

@coveralls
Copy link

Coverage Status

Coverage remained the same at 61.337% when pulling f8837ee on syxolk:wix-packaging into b06ab54 on mp4096:master.

@mp4096
Copy link
Owner

mp4096 commented May 28, 2017

Thanks! Python 2 works well, I've just tested it out.

mp4096
mp4096 requested changes May 28, 2017
View reviewed changes
Copy link
Owner

@mp4096 mp4096 left a comment

Choose a reason for hiding this comment

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

Looks good apart from two points:

  • PEP8-compliance in the Python script 🇪🇸 🔥
  • I think we should drop the pandoc dependency and version the LICENSE.rtf file in git. Rationale: Easier building in AppVeyor.

packaging/wix/configure.py Outdated
from os import path
from string import Template
import toml

Copy link
Owner

Choose a reason for hiding this comment

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

Please use two empty lines between functions, cf. PEP8.

packaging/wix/configure.py Outdated
with open(path.join("..", "..", "Cargo.toml")) as f:
cargo = toml.load(f)
return cargo["package"]["version"]

Copy link
Owner

Choose a reason for hiding this comment

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

Two empty lines here as well...

packaging/wix/configure.py Outdated

with open("indentex.wxs", "w") as f:
f.write(tmpl.substitute(version=version))

Copy link
Owner

Choose a reason for hiding this comment

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

...and here too.

packaging/wix/configure.py Outdated
tmpl = Template(f.read())

with open("indentex.wxs", "w") as f:
f.write(tmpl.substitute(version=version))
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe it would be better to pass a context? Something along the lines of tmpl.substitute(**context).

It's more future-proof if someday we have more variables than just the version.

However, don't write any code you don't need right now, so...

packaging/wix/make_installer.ps1 Outdated
@@ -0,0 +1,5 @@
python configure.py
pandoc -f markdown -t rtf ..\..\LICENSE.md -s -o LICENSE.rtf
Copy link
Owner

Choose a reason for hiding this comment

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

The more I think about it... maybe it is better to version LICENSE.rtf? The thing is, I'd like to build a WiX installer in AppVeyor on each tagged commit / release. AppVeyor has Wix 3.11 (as of now) and Python installed by default, but no pandoc.

@syxolk
Copy link
Collaborator Author

syxolk commented May 28, 2017

I think we should drop the pandoc dependency and version the LICENSE.rtf file in git. Rationale: Easier building in AppVeyor.

It's actually pretty easy to install pandoc using Chocolatey: choco install pandoc. Chocolatey is also preinstalled on Appveyor.

https://chocolatey.org/packages/pandoc

@mp4096
Copy link
Owner

mp4096 commented May 28, 2017

Oh, ok. Consider this one resolved!

mp4096
mp4096 approved these changes May 28, 2017
View reviewed changes
Copy link
Owner

@mp4096 mp4096 left a comment

Choose a reason for hiding this comment

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

ACCEPTABLE.

@coveralls
Copy link

coveralls commented May 28, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 61.337% when pulling dc5d7c4 on syxolk:wix-packaging into b06ab54 on mp4096:master.

@mp4096
Copy link
Owner

mp4096 commented May 28, 2017

@syxolk Would you like me to squash on merge?

@syxolk
Copy link
Collaborator Author

syxolk commented May 28, 2017

One last thing: Should we rename the folder from wix to windows? This may make more sense when we add stuff like debian or arch:

- packaging
|- arch
|- debian
|- windows

@mp4096
Copy link
Owner

mp4096 commented May 28, 2017

Hm, I don't know. The thing is, maybe we'd like to make a choco package in the future. Mayber windows_installer or windows_wix?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 61.337% when pulling 8d71f0d on syxolk:wix-packaging into b06ab54 on mp4096:master.

@mp4096 mp4096 merged commit f70da52 into mp4096:master May 28, 2017
@mp4096 mp4096 mentioned this pull request May 28, 2017
Closed
4 tasks
@syxolk
Copy link
Collaborator Author

syxolk commented May 28, 2017

@syxolk Would you like me to squash on merge?

I wanted to say yes but nevermind.

@mp4096
Copy link
Owner

mp4096 commented May 28, 2017 •
edited
Loading

¯\_(ツ)_/¯

@mp4096 mp4096 mentioned this pull request May 28, 2017
Closed
@syxolk syxolk deleted the wix-packaging branch May 29, 2017 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mp4096 mp4096 mp4096 approved these changes

Assignees
No one assigned
Labels
ergonomics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@syxolk @coveralls @mp4096