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

For Discussion: Devcontainer #3108

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

Quafadas
Copy link
Contributor

@Quafadas Quafadas commented Apr 5, 2024

This adds a dev container file which stands up a one click cleanroom development environment in GitHub code spaces.

Stuff that seems to work:

  • Compile
  • IDE / metals

To my surprise, testing seems flaky in this environment ...

X mill.api.PathRefTests.json.ref 20ms 
  utest.AssertionError: assert(json == expected)
  json: String = "ref:v0:89fa1bc2:/tmp/13626813947610103712/foo.txt"
  expected: String = "ref:v0:4c7ef487:/tmp/13626813947610103712/foo.txt"

I think this is one of the quite fundamental tests - I'm surprised it fails here, but not in CI! I think this PR can have value none-the-less, although I'm not sure if I would be able to get mill to the stage where it passes all tests here without (some quite significant) help.

Many of the test suites (run inside the container) do pass, however, so it's at least close-to-right

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Thank you! Since I don't use this feature, I wonder how to ensure it stays up-to-date and working.

Is there some way to automate some testing for this setup file, e.g. automatically build and validate a devcontainer in CI? Is there some tooling to check for updates?

@Quafadas
Copy link
Contributor Author

Hmmmm.... that is... a great point. I'll do some research in the coming days.

@Quafadas
Copy link
Contributor Author

Quafadas commented May 21, 2024

@lefou I evolved this is a little after some extra research and taking a little step back to think "what I would want" too.

In respect of your comment about CI - I added an additional workflow
https://github.com/Quafadas/mill/blob/codespace/.github/workflows/container.yml

Which rebuilds the container on push to main. It checks only that mill __.compile is successful in the container and that that command exits with a 0.

i.e. assumes that a merge to main compiles. This is quite a low baseline, but I think is about right for what we want, as all the "real" testing is done elsewhere... we only need a basic check for the container itself.

According to the docs, that workflow should also cache that container, so that a cold start doesn't need to build the environment from scratch.
https://github.com/devcontainers/ci/blob/main/docs/github-action.md

In the dev container itself, I also added coursier and millw...
https://github.com/Quafadas/mill/blob/codespace/.devcontainer/features/mill/install.sh

So that one can type in cs, and mill to the command line and they work straight up from a cold start. Then, installed metals dependancies via coursier, so that metals has it's dependancies when the container is instantiated.

Finally, when the container is created, this runs.
onCreateCommand": "mill -j 0 __.prepareOffline && mill -j 0 __.compile && mill mill.bsp.BSP/install && mill --import ivy:com.lihaoyi::mill-contrib-bloop: mill.contrib.bloop.Bloop/install

I hope that again, this reduces "cold start" time for a contributor by getting mills dependancies. It puts the first compile in place and gets the BSP support ready. I'm not sure if it's easily possible to do more than this - I hope it helps as those steps were quite time consuming first time out.

Currently, CI in this repo is red (I think one of the test suites are flaky and times out)?

On my fork, the CI is also red :-(, I believe because the job doesn't have permission to add this container to mills artefact repository.
https://github.com/Quafadas/mill/actions/runs/9157845979/job/25175008941

Which ... seems reasonable. I don't really know how to make this green though without merging to main, as I don't think my fork should have those permissions? I'd welcome advice here...

In that job, the container building does appears successful, and that configuration appears to have worked successfully in one of my many toy projects...

https://github.com/Quafadas/live-server-scala-cli-js/blob/main/.github/workflows/container.yml
https://github.com/Quafadas/live-server-scala-cli-js/actions/runs/9155999012

One final consideration: as the container is cached, I fear it may contribute to the resource usage of the com.lihaoyi ecosystem. I would not wish to inadvertently create a problem here.

I have written a bible. I am sorry... if there are parts of this you don't like, I'm happy to adjust further.

@lefou lefou requested review from lihaoyi and lolgab May 22, 2024 16:14
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.

None yet

2 participants