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

BIG CHANGES #46

Merged
merged 4 commits into from
Jan 13, 2017
Merged

BIG CHANGES #46

merged 4 commits into from
Jan 13, 2017

Conversation

swizzlr
Copy link
Contributor

@swizzlr swizzlr commented Jan 12, 2017 •
edited
Loading

@hamin @tianon

I took this opportunity to substantially upgrade the Dockerfile.

  • Improved formatting of package install (hard wrapped errything)
  • Key importing, download and install happen in one layer (as it should be)
  • Suppress output of apt and GPG, which seem to enjoy printing to STDERR when there's no error. The build is now effectively silent.

This checks all the boxes!

@swizzlr swizzlr changed the title Remove a few dependencies, increase security, silence output, decreas… BIG CHANGES Jan 12, 2017
@swizzlr swizzlr mentioned this pull request Jan 12, 2017
Merged
17 tasks
tianon
tianon reviewed Jan 12, 2017
View reviewed changes
Copy link

@tianon tianon left a comment

Choose a reason for hiding this comment

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

A few notes about debuggability, but otherwise looks sane!

Is vim used by the Swift core, or just something that's useful to have in the container? (might be worth leaving that out -- didn't notice it before)

Dockerfile
; do \
gpg --quiet --keyserver ha.pool.sks-keyservers.net --recv-keys "$key" > /dev/null 2>&1; \
done \
&& gpg --batch --verify --quiet swift.tar.gz.sig swift.tar.gz > /dev/null 2>&1 \
Copy link

Choose a reason for hiding this comment

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

Same note about 2>&1 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'm not wedded to it. Better to be able to debug later, most people will consume this as an image, not by building manually.

Dockerfile
# uid Swift 3.x Release Signing Key <[email protected]>
A3BAFD3556A59079C06894BD63BC1CFE91D306C6 \
; do \
gpg --quiet --keyserver ha.pool.sks-keyservers.net --recv-keys "$key" > /dev/null 2>&1; \
Copy link

Choose a reason for hiding this comment

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

Same note about 2>&1 here.

Dockerfile
rsync \
libxml2 \
git \
libcurl4-openssl-dev > /dev/null 2>&1 && \
Copy link

Choose a reason for hiding this comment

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

If this fails, 2>&1 will swallow the error message, right? (makes debugging failures somewhat harder -- I'd rather have easy debugging and a verbose build than a quiet build that's difficult to debug)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re: Vim – not at all required, but having some text editor available was requested by users, and we didn't think it had a significant impact on image size. Would it be better for us to remove it?

@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 12, 2017

Oh, and I got to remove wget in favor of curl. 🍾

@hamin
Copy link
Contributor

hamin commented Jan 12, 2017

@swizzlr @tianon so i'm ok with removing vim. However, I don't think we can remove curl. The reason is that Foundation uses curl for NSURLSession/NSURLRequest underneath. So I think we would still want that and also it's used for Swift's package manager. See #35

tianon
tianon reviewed Jan 12, 2017
View reviewed changes
Dockerfile
make \
libc6-dev \
clang-3.6 \
vim \
Copy link

Choose a reason for hiding this comment

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

Just tested in a bare ubuntu:16.04, and installing just vim is ~57.87 MB, which is space users can't ever reclaim if it's in the base image, whereas the following is pretty trivial:

FROM swift
RUN apt-get update -qq && apt-get install -yqq vim && rm -rf /var/lib/apt/lists/*

Combine that with automated builds (https://docs.docker.com/docker-hub/builds/) and repository links (https://docs.docker.com/docker-hub/builds/#repository-links) and it's reasonably easy to have an up-to-date image built FROM this one with vim pre-installed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@swizzlr what do u think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hamin @tianon agreed. great idea with repo links. let's do this.

tianon
tianon approved these changes Jan 13, 2017
View reviewed changes
Copy link

@tianon tianon left a comment

Choose a reason for hiding this comment

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

🤘

@swizzlr swizzlr merged commit 94a4327 into master Jan 13, 2017
@swizzlr swizzlr deleted the huge-refactor branch January 13, 2017 00:24
@hamin
Copy link
Contributor

hamin commented Jan 13, 2017

Omg amazing thank you :) @tianon . Nice work @swizzlr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hamin hamin hamin left review comments

@tianon tianon tianon approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@swizzlr @hamin @tianon