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

rfc(engine): sandbox only when needed rather than throwing an error #8854

Closed
Alizter opened this issue Oct 5, 2023 · 4 comments · Fixed by #9041
Closed

rfc(engine): sandbox only when needed rather than throwing an error #8854

Alizter opened this issue Oct 5, 2023 · 4 comments · Fixed by #9041
Labels
accepted accepted proposals proposal RFC's that are awaiting discussion to be accepted or rejected
Milestone

Comments

@Alizter
Copy link
Collaborator

Alizter commented Oct 5, 2023

Problem

When a rule is configured to be sandboxed, however the action of the rule does not need sandboxing, we raise the following error message:

            User_error.raise
              ~loc
              [ Pp.text
                  "Rule dependencies are configured to require sandboxing, but the rule \
                   has no actions that could potentially require sandboxing."
              ]

This can appear in some rather cryptic locations such as:

In this situation, it doesn't really make alot of sense that raise a user error for the following reasons:

  1. Because incorrect sandbox configurations are generally out of the users control and can be internal to dune.
  2. We have a better alternative solution which is to simply not sandbox.

Proposal 1

We should modify the behaviour in this case to simply conform to the sandboxing needs. If an action doesn't need to be sandboxed, then we should simply not sandbox it.

Proposal 2

In the aforementioned cases, the action in question was in fact the "empty actions" which is just (progn). Perhaps it is vacuously useful to sandbox in this case and we should just change it?

cc @rgrinberg @snowleopard

@Alizter Alizter changed the title rfc(engine): rfc(engine): sandbox only when needed rather than throwing an error Oct 5, 2023
@Alizter Alizter added the proposal RFC's that are awaiting discussion to be accepted or rejected label Oct 5, 2023
@Alizter
Copy link
Collaborator Author

Alizter commented Oct 27, 2023

This has appeared again when trying to fix #9026. The situation is that an empty (progn) can't be sandboxed, but that's ok and having it default to no sandboxing seems sensible.

@snowleopard
Copy link
Collaborator

I think we should just sandbox such actions. It doesn't seem particularly useful to insist on some sort of optimality. The current behaviour seems pretty annoying (imagine an action often flipping between requiring and not requiring sandboxing).

@Alizter
Copy link
Collaborator Author

Alizter commented Oct 29, 2023

A conservative fix for the issues I've been encountering would be to mark Prog [] as needing sandboxing. This would leave the error for other situations.

@snowleopard
Copy link
Collaborator

I don't understand why we need to be conservative. I'm inclined to just remove the error and do the sandboxing if the sandboxing configuration asks for it.

@rgrinberg rgrinberg added the accepted accepted proposals label Oct 30, 2023
@rgrinberg rgrinberg added this to the 3.12.0 milestone Oct 30, 2023
@rgrinberg rgrinberg linked a pull request Oct 30, 2023 that will close this issue
emillon added a commit to emillon/opam-repository that referenced this issue Nov 28, 2023
CHANGES:

- Introduce `$ dune ocaml doc` to open and browse documentation. (ocaml/dune#7262, fixes
  ocaml/dune#6831, @EmileTrotignon)

- `dune cache trim` now accepts binary byte units: `KiB`, `MiB`, etc. (ocaml/dune#8618,
  @Alizter)

- No longer force colors for OCaml 4.03 and 4.04 (ocaml/dune#8778, @rgrinberg)

- Introduce new experimental odoc rules (ocaml/dune#8803, @jonjudlam)

- Introduce the `runtest_alias` field to the `cram` stanza. This allows
  removing default `runtest` alias from tests. (@rgrinberg, ocaml/dune#8887)

- Do not ignore libraries named `bigarray` when they are defined in conjunction
  with OCaml 5.0 (ocaml/dune#8902, fixes ocaml/dune#8901, @rgrinberg)

- Dependencies in the copying sandbox are now writeable (ocaml/dune#8920, @rgrinberg)

- Absent packages shouldn't prevent all rules from being loaded (ocaml/dune#8948, fixes
  ocaml/dune#8630, @rgrinberg)

- Correctly determine the stanza of menhir modules when `(include_subdirs
  qualified)` is enabled (@rgrinberg, ocaml/dune#8949, fixes ocaml/dune#7610)

- Display cache location in Dune log (ocaml/dune#8974, @nojb)

- Re-run actions whenever `(expand_aliases_in_sandbox)` changes (ocaml/dune#8990,
  @rgrinberg)

- Rules that only use internal dune actions (`write-file`, `echo`, etc.) can
  now be sandboxed. (ocaml/dune#9041, fixes ocaml/dune#8854, @rgrinberg)

- Do not re-run rules when their location changes (ocaml/dune#9052, @rgrinberg)

- Correctly ignore `bigarray` on recent version of OCaml (ocaml/dune#9076, @rgrinberg)

- Add `test_` prefix to default test name in `dune init project` (ocaml/dune#9257, fixes
  ocaml/dune#9131, @9sako6)

- Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting
  of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter)

- [coq rules] Be more tolerant when coqc --print-version / --config don't work
  properly, and fallback to a reasonable default. This fixes problems when
  building Coq projects with `(stdlib no)` and likely other cases. (ocaml/dune#8966, fix
  ocaml/dune#8958, @Alizter, reported by Lasse Blaauwbroek)

- Dune will now run at a lower framerate of 15 fps rather than 60 when
  `INSIDE_EMACS`. (ocaml/dune#8812, @Alizter)

- dune-build-info: when `version=""` is found in a `META` file, we now return
  `None` as a version string (ocaml/dune#9177, @emillon)

- Dune can now be built and installed on Haiku (ocaml/dune#8795, fix ocaml/dune#8551, @Alizter)

- Mark installed directories in `dune-package` files. This fixes `(package)`
  dependencies against packages that contain such directories. (ocaml/dune#8953, fixes
  ocaml/dune#8915, @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted accepted proposals proposal RFC's that are awaiting discussion to be accepted or rejected
Projects
None yet
3 participants