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 CPack support to provide Win32 binaries and github releases #154

Merged
merged 12 commits into from
Nov 6, 2018

Conversation

TcT2k
Copy link
Contributor

@TcT2k TcT2k commented May 16, 2018

Partly as general improvement but also specifically addressing issue #148

  • Add CPack support to package binaries (as ZIPs or tar.gz for now)
    • This would also allow other types of packages later on like Windows/mac installers
  • Collect package as artifacts in appveyor
  • Deploy artifacts as github release like this
  • Cache unicorn build for MSYS build AppVeyor CI: Add MSYS2 Unicorn build to cache #92

Open ToDos (some of which I need input on):

  • Include version number in executable
    • Print during to log
    • Add to SDL window title
  • Decide to always create a release or just for tags
  • Releases in same repo or different one
  • Release naming (filename currently uses git revision, release name uses appveyor automatic "version number")

I'll create issues for a few open things after this is merged:

  • Include MSYS dependencies in package

package.cmake Outdated
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
OUTPUT_VARIABLE GIT_REVISION
OUTPUT_STRIP_TRAILING_WHITESPACE
)
Copy link
Member

Choose a reason for hiding this comment

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

nit: Please use 2 spaces per "tab"

appveyor.yml Outdated
- path: $(BUILD_TYPE)_build/*.zip
name: executable
- path: $(BUILD_TYPE)_build/*.sha256
name: checksum
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we don't really need this?
If we push straigth to github releases the files are likely untempered with, so I don't see the benefit of this.

@JayFoxRox
Copy link
Member

JayFoxRox commented May 16, 2018

Looking good! Thanks a lot for looking into it.

Here are my thoughts:

  • We can add MSYS deployment at a later stage to fast-forward this change, simply because one deployed Windows build is enough (for now); gcc builds will get a lot of testing through Linux and macOS anyway. We'll also continue to run MSYS on CI. Multiple releases might even be confusing to users - this would be resolved when having a custom webfrontend.
  • We should always generate a build, so we can have "nightly" build, even when we don't release a new major update.
  • There should still be major updates from time to time to mark stable versions (to be included in package managers etc.) or announce completion of major features.
  • We should not build PRs for security reasons. If people want cutting-edge features, they should just run their own builds at their own risk. So only the reviewed master-updates or tags should get build.
  • Not sure about which repo to release on. But I think this repo is fine as it has the most appropriate name. Reasons for moving it would be stronger seperation of users/developers, but that seperation can also happen with a webfrontend on our website later.
  • When there's a screenshot, we should always know which version it was from, so we should add the git revision information into the SDL window title (other projects I've worked on also do this).
  • Binary filename should probably stay openswe1r, packagename name should be something like OpenSWE1R-[-dirty].zip ? Release name also something like OpenSWE1R-<git-commit> ? To be honest, it has been a couple of months (/ years?) since I've worked on deployment, so I'm not even sure what our options are.
  • GNU in packagename is confusing, I'd suggest using OpenSWE1R-<git-commit>-x64-Windows-MSVC and OpenSWE1R-<git-commit>-x64-Windows-MSYS. This turns the last part into an optional variant. Linux and macOS would just be called OpenSWE1R-<git-commit>-x64-Linux and OpenSWE1R-<git-commit>-x64-macOS. You'll also notice that I split OS and architecture, because we'll eventually have other architectures probably. So we should be more explicit about it (architecture could also be moved to the end or something - I don't know).
  • (Edit:) Consider naming it "openswe1r" in all lowercase (in general the filename could be all lowercase), so we can make a distinction between the "OpenSWE1R" collection of projects (such as "swe1r-tools" etc.) and the actual rewrite ("openswe1r").

(Also consider joining our gitter.im channel)

Included file following http://editorconfig.org/ to facilitate uniform code formatting
@TcT2k TcT2k force-pushed the cmake_package branch 4 times, most recently from d3c541a to 1af1610 Compare May 17, 2018 08:59
.gitignore Outdated
@@ -1 +1,4 @@
build/
unicorn/
msvc_build/
msys_build/
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather have some wildcard style folder then, something like build/ and build_*/

Our Build Instructions also simply try to keep the unicorn/ folder in the build/ directory.


Unrelated to this PR, here's some background on the unicorn situation:

Ideally this whole unicorn/ thing shouldn't be necessary.
Personally, I considered unicorn not being provided by the platform as a temporary issue.
On most platforms it's provided by the package manager (most Linux distributions, some BSDs, macOS/homebrew, vcpkg); even on MSYS it has a package but they somehow never pushed binaries to the server (#146).
If this problem persists, we should consider optionally submoduling unicorn in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem changing this. It was more or less just to make sure that the version is not marked as "dirty" during build. Maybe I'll just change the appveyor script to use build as it's a new VM for each build type anyway.

TcT2k added 2 commits May 17, 2018 14:06
Version info is added to SDL window title and logged at startup
This enables easy binary distribution for Windows and
might later even be expanded for other platforms and/or installers
@JayFoxRox JayFoxRox merged commit fc13ad3 into OpenSWE1R:master Nov 6, 2018
@JayFoxRox
Copy link
Member

I just merged this as-is. I had intended to do this months ago but got distracted with other projects.
By now I've lost oversight - rather than delaying this further and causing more work, I think it's better to just accept this change. If there's anything broken or horrible it can be fixed iteratively.

Thanks for this large contribution!

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.

2 participants