-
Notifications
You must be signed in to change notification settings - Fork 45
Skip package management if running in github-hosted container #986
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
Conversation
Alternatively, we could also allow using an environment variable (or an input config value?) to allow skipping package management. |
58f18b6
to
ce7b404
Compare
@punchagan Is current code OK for you? |
ce7b404
to
5cf913c
Compare
@smorimoto Thanks for reviewing the PR!
No, the current code checks for the |
b0af8e0
to
1045344
Compare
@smorimoto I pushed a small change to change the path to diff --git a/packages/setup-ocaml/src/unix.ts b/packages/setup-ocaml/src/unix.ts
index 532cffce..19aba5bc 100644
--- a/packages/setup-ocaml/src/unix.ts
+++ b/packages/setup-ocaml/src/unix.ts
@@ -1,6 +1,5 @@
import * as fs from "node:fs/promises";
import * as path from "node:path";
-import * as process from "node:process";
import { exec, getExecOutput } from "@actions/exec";
import { PLATFORM, RUNNER_ENVIRONMENT } from "./constants.js";
@@ -39,7 +38,7 @@ async function skipPackageManagement() {
// github-hosted Docker container.
let isContainerRunner = false;
try {
- const dockerenv = path.join(process.cwd(), ".dockerenv");
+ const dockerenv = path.join("/", ".dockerenv");
await fs.access(dockerenv, fs.constants.R_OK);
isContainerRunner = true;
} catch { |
1045344
to
fdb117b
Compare
Oops, indeed. |
62b6630
to
a473906
Compare
GitHub allows running actions in containers [1] on the runners, and when using a non apt-based Linux distribution, setup-ocaml doesn't work correctly. This commit skips package management on docker containers, similar to the behavior on self-hosted runners. [1]: https://docs.github.com/en/actions/writing-workflows/choosing-where-your-workflow-runs/running-jobs-in-a-container
a473906
to
999cb79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the OCaml setup action to skip system package management when running in GitHub-hosted Docker containers (or self-hosted runners) and updates CI to test in an Arch Linux container.
- Added
skipPackageManagement()
to detect Docker containers and self-hosted environments. - Updated
installUnixSystemPackages
andupdateUnixPackageIndexFiles
to call the new skip helper. - Extended CI with an
archlinux
container job and SSL setup in the workflow.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
packages/setup-ocaml/src/unix.ts | Introduced skipPackageManagement and replaced self-hosted checks |
.github/workflows/workflow.yml | Added test-container job and SSL install step to CI workflow |
Comments suppressed due to low confidence (4)
packages/setup-ocaml/src/unix.ts:35
- [nitpick] There are no tests covering
skipPackageManagement
. Add unit tests for both container and non-container scenarios to verify the skip logic.
async function skipPackageManagement() {
.github/workflows/workflow.yml:65
- This
run
step is indented under thewith
block rather than at the job step level. It needs to be unindented to align with other steps so the workflow executes correctly.
- run: opam install ssl
.github/workflows/workflow.yml:67
- The
test-container
job lacks a step to install thessl
package via opam, which may cause failures. Add a- run: opam install ssl
after setting up OCaml.
test-container:
packages/setup-ocaml/src/unix.ts:1
- fs.constants isn’t exported from the promises-only import, so
fs.constants.R_OK
will be undefined at runtime. Import constants from 'node:fs' or switch toimport * as fs from 'node:fs'
.
import * as fs from "node:fs/promises";
Thank you for the review, the code improvements and the merge! 🎉 |
@smorimoto Is it possible to also publish a release with this change? |
I'm on the plane now, so I'll do it later! |
Okay, thank you! Have a safe flight! |
Done! |
GitHub allows running actions in containers 1 on the runners, and when
using a non apt-based Linux distribution, setup-ocaml doesn't work
correctly. This commit skips package management on docker containers,
similar to the behavior on self-hosted runners.