Skip to content

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

Merged
merged 1 commit into from
Jun 20, 2025

Conversation

punchagan
Copy link
Contributor

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.

@punchagan
Copy link
Contributor Author

Alternatively, we could also allow using an environment variable (or an input config value?) to allow skipping package management.

@smorimoto smorimoto requested a review from Copilot June 17, 2025 16:33
Copilot

This comment was marked as outdated.

@smorimoto smorimoto force-pushed the fix-container-runner branch from 58f18b6 to ce7b404 Compare June 17, 2025 16:58
@smorimoto
Copy link
Member

@punchagan Is current code OK for you?

@smorimoto smorimoto force-pushed the fix-container-runner branch from ce7b404 to 5cf913c Compare June 17, 2025 17:03
@punchagan
Copy link
Contributor Author

@smorimoto Thanks for reviewing the PR!

@punchagan Is current code OK for you?

No, the current code checks for the .dockerenv in the current working directory, but my original PR was looking for /.dockerenv. This is not documented but something I found from the example in the documentation page and it seems to work - see a sample build here with the corresponding workflow file. This is a failing build with the current code in this PR.

@punchagan punchagan force-pushed the fix-container-runner branch 7 times, most recently from b0af8e0 to 1045344 Compare June 19, 2025 06:07
@punchagan
Copy link
Contributor Author

@smorimoto I pushed a small change to change the path to /.dockerenv instead of ${PWD}/.dockerenv. I've also added a container based run to the workflow GitHub action to test this workflow with any future changes too.

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 {

@smorimoto smorimoto force-pushed the fix-container-runner branch from 1045344 to fdb117b Compare June 20, 2025 07:21
@smorimoto
Copy link
Member

Oops, indeed.

@smorimoto smorimoto force-pushed the fix-container-runner branch 2 times, most recently from 62b6630 to a473906 Compare June 20, 2025 07:27
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
@smorimoto smorimoto force-pushed the fix-container-runner branch from a473906 to 999cb79 Compare June 20, 2025 07:29
@smorimoto smorimoto added the enhancement New feature or request label Jun 20, 2025
@smorimoto smorimoto requested a review from Copilot June 20, 2025 07:30
Copy link

@Copilot Copilot AI left a 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 and updateUnixPackageIndexFiles 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 the with 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 the ssl 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 to import * as fs from 'node:fs'.
import * as fs from "node:fs/promises";

@smorimoto smorimoto merged commit 73c794b into ocaml:master Jun 20, 2025
12 checks passed
@punchagan punchagan deleted the fix-container-runner branch June 20, 2025 07:38
@punchagan
Copy link
Contributor Author

Thank you for the review, the code improvements and the merge! 🎉

@punchagan
Copy link
Contributor Author

@smorimoto Is it possible to also publish a release with this change?

@smorimoto
Copy link
Member

I'm on the plane now, so I'll do it later!

@punchagan
Copy link
Contributor Author

I'm on the plane now, so I'll do it later!

Okay, thank you! Have a safe flight!

@smorimoto
Copy link
Member

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants