Dockerfile, circleci: update protoc to 3.6.1, simplify install by kolyshkin · Pull Request #2756 · moby/swarmkit

kolyshkin · 2018-09-26T21:50:11Z

Use unzip options to unpack directly to /usr/local/{include,bin},
to avoid unnecessary I/O.

Unfortunately unzip does not support unpacking from a pipe (due
to a limitation of .zip format -- the index is at EOF) so we still
have to save the .zip to disk, read it back, and remove. If they
could only provide tarballs... sigh

// cc @wk8 @anshulpundir

wk8 · 2018-09-26T22:06:45Z

-  && cd .. && rm -rf $PROTOC_TMP_DIR
+RUN curl --silent --show-error --location --output protoc.zip \
+  https://github.com/google/protobuf/releases/download/v$PROTOC_VERSION/protoc-$PROTOC_VERSION-linux-x86_64.zip \
+  && unzip protoc.zip -x readme.txt -d /usr/local \


I kinda preferred the explicit mvs above, since it at least ensured we didn't copy whatever was in the archive, but only some files we cared about. IMO that outweighs whatever I/O we waste with the current version?

There is nothing else in there (except for the readme.txt which we skip):

Archive: protoc.zip creating: /usr/local/include/google/ creating: /usr/local/include/google/protobuf/ inflating: /usr/local/include/google/protobuf/struct.proto inflating: /usr/local/include/google/protobuf/type.proto inflating: /usr/local/include/google/protobuf/descriptor.proto inflating: /usr/local/include/google/protobuf/api.proto inflating: /usr/local/include/google/protobuf/empty.proto creating: /usr/local/include/google/protobuf/compiler/ inflating: /usr/local/include/google/protobuf/compiler/plugin.proto inflating: /usr/local/include/google/protobuf/any.proto inflating: /usr/local/include/google/protobuf/field_mask.proto inflating: /usr/local/include/google/protobuf/wrappers.proto inflating: /usr/local/include/google/protobuf/timestamp.proto inflating: /usr/local/include/google/protobuf/duration.proto inflating: /usr/local/include/google/protobuf/source_context.proto inflating: /usr/local/bin/protoc

I have also rechecked it with a latest version, 3.6.1:

Archive: protoc.zip creating: /usr/local/include/google/ creating: /usr/local/include/google/protobuf/ inflating: /usr/local/include/google/protobuf/wrappers.proto inflating: /usr/local/include/google/protobuf/field_mask.proto inflating: /usr/local/include/google/protobuf/api.proto inflating: /usr/local/include/google/protobuf/struct.proto inflating: /usr/local/include/google/protobuf/descriptor.proto inflating: /usr/local/include/google/protobuf/timestamp.proto creating: /usr/local/include/google/protobuf/compiler/ inflating: /usr/local/include/google/protobuf/compiler/plugin.proto inflating: /usr/local/include/google/protobuf/empty.proto inflating: /usr/local/include/google/protobuf/any.proto inflating: /usr/local/include/google/protobuf/source_context.proto inflating: /usr/local/include/google/protobuf/type.proto inflating: /usr/local/include/google/protobuf/duration.proto inflating: /usr/local/bin/protoc

I suppose the creators of this .zip file are taking care of what's in there 😆

Doesn't mean there'll never be anything else :) Sure, it's unlikely, but then for such a small benefit, why bother?

That being said, I don't feel super strongly about this, so as long as @dperny is okay with it, why not!

@kolyshkin actually looking at unzip's man page:

SYNOPSIS unzip [-Z] [-cflptTuvz[abjnoqsCDKLMUVWX$/:^]] file[.zip] [file(s) ...] [-x xfile(s) ...] [-d exdir] [...] [file(s)] An optional list of archive members to be processed, separated by spaces. (VMS versions compiled with VMSCLI defined must delimit files with commas instead. See -v in OPTIONS below.) Regular expressions (wildcards) may be used to match multiple members; see above. Again, be sure to quote expressions that would otherwise be expanded or modified by the operating system.

so let's white-list the bin and the includes instead of blacklisting the readme?

make sense @wk8; patch updated

Verified the list is the same as before:

Archive: protoc.zip creating: /usr/local/include/google/ creating: /usr/local/include/google/protobuf/ inflating: /usr/local/include/google/protobuf/wrappers.proto inflating: /usr/local/include/google/protobuf/field_mask.proto inflating: /usr/local/include/google/protobuf/api.proto inflating: /usr/local/include/google/protobuf/struct.proto inflating: /usr/local/include/google/protobuf/descriptor.proto inflating: /usr/local/include/google/protobuf/timestamp.proto creating: /usr/local/include/google/protobuf/compiler/ inflating: /usr/local/include/google/protobuf/compiler/plugin.proto inflating: /usr/local/include/google/protobuf/empty.proto inflating: /usr/local/include/google/protobuf/any.proto inflating: /usr/local/include/google/protobuf/source_context.proto inflating: /usr/local/include/google/protobuf/type.proto inflating: /usr/local/include/google/protobuf/duration.proto inflating: /usr/local/bin/protoc

wk8 · 2018-09-27T00:28:45Z

+# download and install protoc binary and .proto files
+RUN curl --silent --show-error --location --output protoc.zip \
+  https://github.com/google/protobuf/releases/download/v$PROTOC_VERSION/protoc-$PROTOC_VERSION-linux-x86_64.zip \
+  && unzip -d /usr/local protoc.zip include/\* bin\/* \


Minor nitpick: I'd restrict that to bin/protoc, but feel free to ignore :)

IMO, it is actually better to use /usr/local folder same way than it is on CircleCI config.

I'd restrict that to bin/protoc

I can imagine some future version of protoc which calls another binary bundled in the same zip, let's say protoc-helper, and would not work without it, leaving a cryptic message, let's say execve: no such file or directory. With that in mind, I'd rather leave bin/\* in there.

Oh and I just spotted a bug here: it was bin\/* but should be bin/\*. Fixed.

olljanat · 2018-09-29T22:25:29Z

@kolyshkin This sounds good idea and especially migrate to Protobuf 3.6.1 as 3.5.0 version does not work anymore.

I did cherry-pick you commit to #2758 but I also needed to do same modifications to CircleCI config config to make it working. So can you include olljanat@7f8d217 to this PR?

kolyshkin · 2018-10-01T22:19:01Z

So can you include olljanat/swarmkit@7f8d217 to this PR?

Right, I should have made changes to circleci config as well. Commit updated.

For the time being I omitted the nasty chmod 777, let's see if it will work without it (I guess that depends on umask setting in circleci environment)...

olljanat · 2018-10-02T17:16:59Z

@dperny can you check this one so I can drop Protoc update from my PR ( #2758 ) ?

Build looks failing but it look like that it have been issue on build server, not caused by this change so most probably it will work after rebuild.

olljanat · 2018-10-04T16:12:29Z

@kolyshkin can you change example commit text little bit and push it here so it will trigger build running again.

wk8 · 2018-10-04T16:23:20Z

+RUN curl --silent --show-error --location --output protoc.zip \
+  https://github.com/google/protobuf/releases/download/v$PROTOC_VERSION/protoc-$PROTOC_VERSION-linux-x86_64.zip \
+  && unzip -d /usr/local protoc.zip include/\* bin/\* \
+  && rm -f protoc.zip


Arguably this could be put into an install_protoc script that could then be called both by this Dockerfile and Circle

I would agree; maybe put it to direct.mk?

The only problem is I'm waiting for #2750 to be merged first as it modifies Makefiles and I'd like to avoid rebasing )

Agreed on both accounts, reviewing 2750!

While at it, let's simplify installation Use unzip options to unpack directly to /usr/local/{include,bin}, to avoid unnecessary I/O. Unfortunately unzip does not support unpacking from a pipe (due to a limitation of .zip format -- the index is at EOF) so we still have to save the .zip to disk, read it back, and remove. If they could only provide tarballs... *sigh* [v2: update circleci config as well] [v3: add chmod a+r to circleci] Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2018-10-04T16:51:31Z

can you change example commit text little bit and push it here so it will trigger build running again.

done

codecov · 2018-10-04T17:05:51Z

Codecov Report

Merging #2756 into master will increase coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #2756     +/-   ##
=========================================
+ Coverage   61.66%   61.77%   +0.1%     
=========================================
  Files         134      134             
  Lines       21890    21890             
=========================================
+ Hits        13499    13523     +24     
+ Misses       6932     6908     -24     
  Partials     1459     1459

dperny · 2018-10-04T20:55:21Z

yeah LGTM,

wk8 reviewed Sep 26, 2018

View reviewed changes

kolyshkin force-pushed the no-io branch from 98c22bf to 77ce77e Compare September 27, 2018 00:22

wk8 approved these changes Sep 27, 2018

View reviewed changes

kolyshkin force-pushed the no-io branch from 77ce77e to fbcfde1 Compare October 1, 2018 22:17

kolyshkin force-pushed the no-io branch 4 times, most recently from bcf4bbc to e6d714f Compare October 2, 2018 00:39

kolyshkin changed the title ~~Dockerfile: simplify procobuf install~~ Dockerfile, circleci: simplify protoc install Oct 2, 2018

kolyshkin changed the title ~~Dockerfile, circleci: simplify protoc install~~ Dockerfile, circleci: update protoc to 3.6.1, simplify install Oct 2, 2018

olljanat mentioned this pull request Oct 4, 2018

Fix typo removable #2760

Closed

wk8 approved these changes Oct 4, 2018

View reviewed changes

kolyshkin force-pushed the no-io branch from e6d714f to 1831e6f Compare October 4, 2018 16:51

dperny merged commit 01966d4 into moby:master Oct 4, 2018

thaJeztah mentioned this pull request Apr 1, 2019

[17.06 backport] switch CircleCI 2.0, bump go version #2844

Merged

13 tasks

Conversation

kolyshkin commented Sep 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olljanat commented Sep 29, 2018

Uh oh!

kolyshkin commented Oct 1, 2018

Uh oh!

olljanat commented Oct 2, 2018

Uh oh!

olljanat commented Oct 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wk8 Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Oct 4, 2018

Uh oh!

codecov Bot commented Oct 4, 2018

Codecov Report

Uh oh!

dperny commented Oct 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wk8 Oct 4, 2018 •

edited

Loading