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

Build: Add maximum compression #4229

Closed
wants to merge 2 commits into from

Conversation

rasa
Copy link
Member

@rasa rasa commented Jun 26, 2017

Purpose

Add maximum compression to .tar and .zip files.
Removed per @calmh:
Remove /etc files from zip file, as they are unneeded on Windows.
Fix tar builds on Windows, as some Windows users might prefer tars over zips (I do).
Add 'all' option to tar and zip targets, so all executables can be shipped, if desired.

Testing

Results of go run build.go zip before and after (1.1% improvement):

-rw-r--r-- 1 ross 197121  5694763 Jun 26 13:37 syncthing-windows-amd64-v0.14.31-rc.1+5-gdb1dc998.zip
-rw-r--r-- 1 ross 197121  5629660 Jun 26 13:38 syncthing-windows-amd64-v0.14.31-rc.1+9-g7a9e7183-build-max-compression.zip

Authorship

Ross Smith II (rasa) ross@smithii.com

@calmh
Copy link
Member

calmh commented Jun 27, 2017

Hi! Thanks. In principle I'm fine with this, apart from excluding etc and extra from Windows. The extra stuff includes documentation. The etc stuff may currently be useless, but it's not guaranteed to be so in the future and it's equally useless for some other platforms we build for.

@calmh
Copy link
Member

calmh commented Jun 27, 2017

Also clean up the comments on the all target to explain what happens with binaries instead of the current "can't build archives" thing.

@rasa
Copy link
Member Author

rasa commented Jun 27, 2017

@calmh Thanks for reviewing, and considering this PR. I removed the exclusion of the /etc folder, and fixed the 'all' comment. Let me know if there's anything else needed. Thanks!

build.go Outdated
@@ -63,8 +65,8 @@ type archiveFile struct {

var targets = map[string]target{
"all": {
// Only valid for the "build" and "install" commands as it lacks all
// the archive creation stuff.
// Only valid for the "build", "install", "tar" and "zip" commands
Copy link
Member

Choose a reason for hiding this comment

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

Actually, why not add the required stuff to this target, maybe keep the later magic to add all binaries automatically, instead of reusing the syncthing target? The filenames etc will make more sense that way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that makes more sense. Fixing now...

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I started to hack on it, and I'm not sure the best way forward, sorry.

@calmh
Copy link
Member

calmh commented Jul 7, 2017

Maybe scale it back to just the compression changes and add the other stuff at a later time when appropriate.

@rasa
Copy link
Member Author

rasa commented Jul 16, 2017

OK, I backed out the build all changes. Let me know if this fits the bill.

@rasa rasa changed the title Build: Add maximum compression, add 'tar all' option Build: Add maximum compression Jul 16, 2017
@calmh
Copy link
Member

calmh commented Jul 17, 2017

@st-jenkins retest this please

@calmh
Copy link
Member

calmh commented Jul 17, 2017

@st-review merge it

build: Use maximum compression when archiving

@calmh
Copy link
Member

calmh commented Jul 17, 2017

@rasa Thanks!

@st-review
Copy link

👌 Merged as 0ca2ed7. Thanks, @rasa!

@st-review st-review closed this Jul 17, 2017
st-review pushed a commit that referenced this pull request Jul 17, 2017
@rasa rasa deleted the build-max-compression branch July 19, 2017 14:36
viable-hartman pushed a commit to viable-hartman/syncthing that referenced this pull request Aug 25, 2017
@st-review st-review added the pr-merged Legacy label used in the past for pull requests merged to the main tree label Jan 15, 2018
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Jul 18, 2018
@syncthing syncthing locked and limited conversation to collaborators Jul 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion pr-merged Legacy label used in the past for pull requests merged to the main tree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants