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

Let us template the Dockerfiles #6

Closed
wants to merge 2 commits into from
Closed

Let us template the Dockerfiles #6

wants to merge 2 commits into from

Conversation

phadej
Copy link
Contributor

@phadej phadej commented Nov 7, 2019

No description provided.

@phadej phadej requested review from psftw and hvr November 7, 2019 20:36
generate.hs Outdated
{-# LANGUAGE OverloadedStrings, RecordWildCards #-}
-- | Script to generate dockerfiles. Obviously using Haskell.
--
-- If you have @cabal-env@ installed, do:
Copy link
Member

Choose a reason for hiding this comment

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

what about declaring the deps via a cabal metadata block?

i.e. https://cabal.readthedocs.io/en/latest/nix-local-build.html#cabal-v2-run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can cabal v2-repl pick up them as well? as far as it cannot, the development experience is very poor. With cabal-env I can ghci/ghcid the module.

Copy link
Contributor Author

@phadej phadej Nov 8, 2019

Choose a reason for hiding this comment

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

this issue: haskell/cabal#6149

EDIT: the dependency solving on each iteration just takes too long time for the development. Fine when you run it once, but now this script will be (hopefully) evolving.

@phadej
Copy link
Contributor Author

phadej commented Nov 13, 2019

@psftw any opinions on this?

Copy link
Contributor

@psftw psftw left a comment

Choose a reason for hiding this comment

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

I'm not against templating, but I don't think it is justified yet. There should also be instructions on how to run this in a consistent way, ideally without needing to bootstrap additional tooling.

8.8/Dockerfile Outdated
@@ -1,5 +1,5 @@
## Dockerfile for a haskell environment
FROM debian:stretch
FROM buildpack-deps:stretch
Copy link
Contributor

Choose a reason for hiding this comment

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

need to be clear about why this was changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

buildpack-deps seems to be the source for all/most other non-slim official images. It includes most common development tools and c-dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

I carefully reviewed all packages that get installed in this image, and I'm aware that some overlap with buildpack-deps. Before merging a change like this I would like to know exactly what it gets us, and for the overlapping package installs to be removed from this Dockerfile to avoid confusion. I could look it up and do this analysis, but the burden is on you to make the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

slim version uses your list, non-slim uses buildpack-deps which has all the packages except the libtifno-dev.

I think libtinfo-dev should be pulled in by ghc packages, @hvr is it still not the case?


RUN apt-get update && \
apt-get install -y --no-install-recommends gnupg ca-certificates dirmngr curl git && \
echo 'deb http://downloads.haskell.org/debian stretch main' > /etc/apt/sources.list.d/ghc.list && \
Copy link
Contributor

Choose a reason for hiding this comment

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

"stretch" is probably intended to be a template variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

@phadej
Copy link
Contributor Author

phadej commented Nov 20, 2019

I pushed a new variant, I still need to write the instructions how to regenerate.

I think with strectch / buster; slim / non-slim; and n-versions templating is justified.

EDIT: note, this already includes buster images, as there aren't yet upstream .debs.

@phadej phadej requested review from psftw and hvr November 20, 2019 09:12
@psftw
Copy link
Contributor

psftw commented Nov 20, 2019

Taking a quick pass at the latest iteration --

8.4.x is effectively frozen, so let's not make any changes there. Per docs,

only the two most recent minor releases will receive updates

I'm strongly against downgrading to checksums when we have GPG keys. Related: #4

I'm souring on the direction these changes are taking. There is a history of being conservative and avoiding unnecessary complexity in this project. GHC is not released very often and we have only needed to maintain two Dockerfiles at any given time. Before releases I run the images through an informal battery of tests and review the results manually. This has scaled well enough to date without any major hiccups, but won't suffice as we expand to more platforms/variants. A more formal CI suite is in order.
In the latest iteration of this PR (ignoring 8.4) you are expanding to eight image variants which are generated by 90 lines of code, and that code is effectively performing a few string substitutions. You also introduce your personal templating library with an initial commit of last week. I appreciate the enthusiasm, but I hope you can understand that this would introduce a significant barrier to contributors.
I'm unconvinced on the necessity of more than one image per GHC-distro-arch combo. There should be a compelling reason for each variant to exist, not just the fact that it could exist. I've been pretty clear that I want to know exactly what packages are going in to the image (which can be validated with the output of dpkg -l), so hand-waving on the justification for switching to buildpack-deps is not good enough.

OK, hope this doesn't come across too harshly. I really am open to expanding the matrix and using Haskell to do the Dockerfile generation, but it will need to be done in the right spirit of the project as outlined above.

@phadej
Copy link
Contributor Author

phadej commented Nov 20, 2019

You also introduce your personal templating library with an initial commit of last week.

Which uses jinja2 inspired syntax, which is well known. I can change to mustache if it feels better.

OK, hope this doesn't come across too harshly. I really am open to expanding the matrix and using Haskell to do the Dockerfile generation, but it will need to be done in the right spirit of the project as outlined above.

Well, it did.

You asked for maintenance help, I offered some.


8.4.x is effectively frozen, so let's not make any changes there.

I didn't know that. From my perspective I would need images with newest versions of tools (cabal-install and stack and possibly others), but still providing old compilers. Perfectly even down to ghc-7.0. If that's against official policy, then these images are not for me.

Feel free to remove me from this repository.

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.

3 participants