-
Notifications
You must be signed in to change notification settings - Fork 9
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
Earthfile suggestions #24
Comments
Thank you @TravisCardwell ! Almost all of your changes make a lot of sense to me, so I'd be more than happy to incorporate it into the project. I'm adding my comments on a few sections; that means that I completely agree with the changes I haven't replied to :).
This is a good point. I agree with an extra "latest" tag. However, that doesn't correspond to them automatically getting security updates, as we only update this repository once every GHC version or so, so the "latest" tag won't be updated as often we'd like for security purposes. I feel like there are a few things moving here:
Currently, the items 1. and 2. are reflected in our versioning schema, but the number 3. is missing. But I'd argue that number 3. is the most important one in terms of security updates. So, I think ideally we'd also include a timestamp on our tags, have a periodic (maybe weekly) CI process which builds and pushes fresh images with latest packages, and the But let's discuss this on the issue #23, as it seems like a bigger change.
I agree, your version of the Earthfile does indeed look cleaner. I think the slight advantage of the previous version was that it'd reuse the
I completely agree with this. I also read your writeups, so my apologies that use of Earthly was not very well documented here. I think we ought to add both some code comments on our Earthfile, and at least mention how to use Earthly on our README. But, we do not need to wait for documentation before getting these changes in, as they are already a good improvement to the existing codebase. So, I'd be happy to get a PR in with the As you can see, I usually only have a look at the PR's over the weekend, so in case this becomes a bottleneck for you I added you as a collaborator, so feel free to get these changes straight in. Going forward, if you intend to keep maintaining this project we should spend some time to move this away from the DockerHub repository under my name so you can push the images, and add a CI process. Let me know if you're interested in tackling those together :). |
Oh also, do you mind if we put a link in our README to your blog articles on how you got |
Thank you very much for the feedback! Add
|
This change implements the changes discussed in issue #24. Minimal changes to the documentation are made so that it is consistent with the new `Earthfile`. More significant documentation changes will be made in a separate commit.
I pushed a commit with the updated I am going to work on the documentation next. |
Thanks @TravisCardwell ! That all sounds great. As the changes shouldn't cause an interface change, I say there's no need to release a new version; but do let me know if you want me to push a new release. |
I wrote my own version of the
Earthfile
, to learn Earthly as well as try implementing some new features. I am creating this issue to see which (if any) of the changes you are interested in incorporating into the project.Suggestions
Separate External Targets
The motivation is to be able to easily build specific images, without building
all
images. Users can then test changes against a single GHC version before buildingall
versions, providing faster iteration. There is a target for each GHC version, areadme
target for updatingREADME.md
, as well as theall
target that builds everything.For example, the following command can be used to build and test the GHC 9.2.2 image.
Note that these targets do not need to run in separate containers. A top-level
FROM
command is used, and none of these "external" targets have aFROM
command.Upgrade Alpine Packages
The Alpine package index is updated and installed packages are upgraded. #23
Add
latest
TagsThe
ghc-musl
images use tags that specify a version number. (I useGHC_MUSL_VERSION
as the variable name for clarity.) This is great for making repeatable builds.I think it would be a helpful to also create tags that point to the latest image for each GHC version. The tag names could simply leave out the version (
ghc922
) or use the stringlatest
(latest-ghc922
). Doing this allows downstream projects to use new versions (security updates) without having to bump the version number in their configuration.Note that this is not implemented in the
Earthfile
below.Use Latest Cabal Version
Each release of GHC specifies the minimum version of Cabal that it works with, but new versions of Cabal can be used with old version go GHC. Installing the minimum version is useful for testing compatibility, but
ghc-musl
is for building static executables, not testing. I think that it is preferable to use the latest version of Cabal for all GHC versions. There are multiple benefits:cabal list-bin
can be performed. I added similar tests for Stack, btw.Earthfile
is somewhat simplified.Minimize Layers
I try to minimize the number of layers in output images. All consecutive
RUN
commands are combined.Note that the test targets are not output, so they may use separate
RUN
commands for clarity.Minimize Internal Targets
I combined some internal targets (such as
ghcup
,ghc-deps
, andresult
) because there is no practical reason for them to be separate. IMHO, it makes theEarthfile
a bit easier to read and understand.Add
TEST_CABAL
AndTEST_STACK
FlagsI added
TEST_CABAL
andTEST_STACK
flags so that these tests can be turned off using--build-arg
options. For example, the following command can be used to disable Stack tests in CI.Note that the
ghc
target contains everything for an image, while theimage
target outputs the image only after the tests have passed.Use Common Alpine Version
Unless there is a good reason to use different Alpine versions for different images, the version can be specified once.
I specify it at the top-level. Changing this versions invalidates the cache for all targets.
Use
ARG
To ConfigureBASE_TAG
The
BASE_TAG
should be anARG
, as it is not an environment variable.Users who test local builds (adding dependencies necessary for their project, for example) are able to specify a custom
BASE_TAG
using a--build-arg
option. This ensures that they do not overwrite official images.Sort Packages
I prefer to sort the packages installed in the
base-system
target, so that they are easy to maintain. The first installation command, which installs general system software requirements, lists packages sorted by package name. The second installation command, which installs requirements for building static executables with various dependencies, lists packages in groups so that the program, library,-dev
, and-static
packages are kept together on the same line, and the lines are sorted.Use GHCUp
--set
OptionI use the
--set
option instead of separateset
commands, just to reduce the lines of code.Reduce Whitespace
I wrote the
Earthfile
using a different style, which has less whitespace. I think that it makes the file easier to read, but I of course do not mind keeping the current style if it is preferred.Note that the example
Earthfile
s use two-space indentation.Specify Earthly Version
The
VERSION
command will be required in the future. I think it would be a good idea to start specifying it now.Earthfile
I am pasting my
Earthfile
instead of linking to a branch or gist because a branch or gist may be deleted in the future. I am not including comments so that it is easier to read, but I think adding some documentation in comments (or a separate file in the repo) could help people who are not familiar with Earthly.The text was updated successfully, but these errors were encountered: