-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: Allow automatic install of system dependencies (needs opam 2.1) #74
Conversation
Hi @palmskog, thanks for your suggestion! LGTM (albeit users that override the Just 2 questions:
Lines 76 to 78 in e717ad9
& Lines 221 to 223 in e717ad9
|
@erikmd I updated README.md to mention I would personally be in favor of having a
If this is what you had in mind, I can add it to this PR. Finally, do we want to write about depext and confirm-level stuff in a sentence of two in README.md? It looks like the only official documentation is this page, so maybe we should link that? |
Hi @palmskog,
Thanks.
Exactly! I agree with you that it's worth it to have this feature working "out-of-the-box"… If you can add this line Meanwhile, I think you should replace this line Line 663 in e717ad9
with a commented-out version:
and likewise for:
Good idea: I suggest you add some sentences in this section: https://github.com/coq-community/docker-coq-action#opam and the link you suggest looks fine to me. |
BTW, which order looks best to you?
or
Maybe the second one? |
….yml and update documentation
I like the second order best, and I tried this out in the bertrand project, CI log can be seen here. Looks like it works fine, so I added the apt-get command and documentation with release notes link in d6a62ce. |
Looks very good to me. I'm going to squash-merge and doing a release. Thanks a lot @palmskog for your help on this! |
@erikmd but I noticed now I missed your suggestions to comment out the |
Ah OK! no worries, I can do the merge later on tonight |
…efore_script example
OK, documentation update on |
OK thanks @palmskog. Just before merging, I believe I'll add a small unit test in the docker-coq-action CI testsuite. Namely, a package that would rely on a system package such as |
FTR, given the following command:
so… let's stick to
|
A unit test sounds like a good idea. However, isn't |
Yes, see the end of my comment :) Do you think |
|
`custom_image / docker-coq / opam / auto install depexts`
Since all Docker images switched to opam 2.1, it might make sense to enable installing depext dependencies automatically (in this case, Debian system packages).
However, the current definition of the
install
task does not permit installing depext dependencies, since a simple-y
/--yes
does not suffice for system packages, instead one has to:Note that Docker-Coq-Action users will still have to run
sudo apt-get update -y -q
somewhere before the dependency installation to get system packags, but this is already documented in the wiki. The--confirm-level=unsafe-yes
is much more obscure to figure out and set manually, so I propose we set it by default here.