-
Notifications
You must be signed in to change notification settings - Fork 7k
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
PLT-4068: Add .zip to build process & convert config.json to use CRLF #4219
PLT-4068: Add .zip to build process & convert config.json to use CRLF #4219
Conversation
Fantastic, thanks @shieldsjared!! Queuing for review, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! I had no idea there was a zip command to do the conversion.
@@ -333,9 +333,13 @@ else | |||
cp $(GOPATH)/bin/windows_amd64/platform.exe $(DIST_PATH)/bin # from cross-compiled bin dir | |||
endif | |||
@# Package | |||
tar -C dist -czf $(DIST_PATH)-$(BUILD_TYPE_NAME)-windows-amd64.tar.gz mattermost | |||
ifeq (, $(shell which zip)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I like the fallback. I would much rather have it fail then produce inconsistent output. The make package
target is designed for our build machine and I would much rather have it fail here then when it's trying to upload.
Also nobody merge this. I probably need to modify the build machine for this to work. |
@crspeller - no worrries, I'll update it tonight. |
@shieldsjared yeah but I didn't take into account what @crspeller is saying which makes a lot of sense, maybe we should just force the windows package to be a zip file |
No worries! Just wanted to ensure you saw it. I'm happy to do whatever y'all think is best. |
@shieldsjared Waiting on your changes to merge this. |
Shoot!! I'll get it done in just a little bit! Sorry about that |
bbda84a
to
b924f37
Compare
…e, instead of a TAR. Included with ZIP is the flag to convert LF to CRLF.
89b0238
to
5e7dee7
Compare
@crspeller - Sorry about the delay! Completely lost sight of this. (I'll blame remediation of issues w/ the house after Hurricane Matthew!). I removed the fallback from the target... The reason it's moving INTO the directory is because the zip command doesn't have way to target the internal folder structure w/in the archive (like tar does).... So I had to do that to ensure the resulting ZIP matches the same structure. If you want to spit out a ZIP from the 3.4 release and let me know where to download, I can replace my production team-instance w/ the ZIP contents to ensure that the CRLF conversions don't create any issues. If it does, they should be readily apparent. That'll give us end-to-end confidence since the build-server will ultimately be the producer. |
@shieldsjared Link to the master build with this PR merged for you to try out: https://releases.mattermost.com/mattermost-platform/master/mattermost-enterprise-linux-amd64.tar.gz |
…e, instead of a TAR. Included with ZIP is the flag to convert LF to CRLF. (#4219)
Summary
Modifies the
makefile
to generate a windows-friendly archive (e.g. zipfile) instead of a tar file for the windows distribution. Also, to align w/ windows-friendliness, a flag is included in the zip operation to convert LF's to CRLF's (which will enable an easier experience when updatingconfig.json
.Note that if the system executing does not have the
zip
command/package available, the build will gracefully fallback to the standard tar packaging.Ticket Links
PLT-4068: Add .zip to build process for Windows and update config.json so it renders properly in Notepad
DOCS/PR-483: Windows Installation/Deployment Guide
Checklist