Skip to content

Expand Sys builtins - #434

Merged
gares merged 2 commits into
LPCIC:masterfrom
shonfeder:sys-builtins
Aug 10, 2026
Merged

Expand Sys builtins#434
gares merged 2 commits into
LPCIC:masterfrom
shonfeder:sys-builtins

Conversation

@shonfeder

Copy link
Copy Markdown
Contributor

This aims at expanding the available builtins for interacting with the OS, as proposed in #427. I an opening this initial PR with just 6 additions, so I can gather feedback on whether the general approach and style is in line with your dev preferences.

I am happy to have this merged as is, if you are happy with it and would like contributions around this size (perhaps some tests should also be added? Could you point me to the right spot for those?). But I wold also be happy to gather feedback on the changes staged here, and then finish FFI for the following functions from Sys:

    external chdir : string -> unit = "caml_sys_chdir"
    external mkdir : string -> int -> unit = "caml_sys_mkdir"
    external rmdir : string -> unit = "caml_sys_rmdir"
    external getcwd : unit -> string = "caml_sys_getcwd"

@gares gares left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems some api need recent ocaml. I think we could document that, and use ppx optcomp to have the only in newer versions

@shonfeder

Copy link
Copy Markdown
Contributor Author

Ah! I didn't see that about Sys.is_regular_file, we could skip adding that for now too? It's not really essential IMO. I'd be happy with just dropping that only sticking with stuff that is compatible with the current lower ocaml bound. WDYT?

@gares

gares commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Ok drop it

Comment thread src/builtin.ml
@shonfeder
shonfeder marked this pull request as draft August 1, 2026 21:27
Comment thread src/builtin.elpi Outdated
@shonfeder

Copy link
Copy Markdown
Contributor Author

I am just reworking the doc comments. I have put together some crude tests in shonfeder@f1e9d47 . This is enough to convince me that all the additions are working as expected, but they are not suitable for inclusion in the repo as is, because they do no sandboxing of the file system mutations. I could not find an existing mechanism for this within your current test harness (tho running the tests thru dune rules rather than make could do this for us). If you're happy merging without the tests, that's OK with me. If you'd like the tests, please advise on whether you have a preferred sandboxing strategy.

@shonfeder
shonfeder marked this pull request as ready for review August 3, 2026 01:36
@shonfeder

Copy link
Copy Markdown
Contributor Author

Should be ready for another look and merge if you're happy with it!

If tests are desired for this, dune cram tests could be a good fix, since all the functionality requires interaction with the OS environment.

@gares

gares commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Now that I think about it twice, I think you should name all sys functions sys.* A proxy can be provided if clients break due to the renaming of getenv, command, gettimeofday. Maybe the latter belongs to unix?

Maybe you can start putting the new code in sys and do the renaming of existing code in another pr.

@gares

gares commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Also, because of another merge, you should re-promote the tests.
It would also be nice to have a few lines in the CHANGES file.

- Add a helper for conditionally constructing error diagnostics
- add builtins for readdir, file_exists, is_directory, remove,
  rename, chdir, mkdir, rmdir, getcwd, and readdir
@shonfeder

Copy link
Copy Markdown
Contributor Author

Now that I think about it twice, I think you should name all sys functions sys.* A proxy can be provided if clients break due to the renaming of getenv, command, gettimeofday. Maybe the latter belongs to unix?

Maybe you can start putting the new code in sys and do the renaming of existing code in another pr.

I agree that this naming is better! That is actually what I am doing in the project I pulled these from, but I tried to adopt the existing convention here. I've made this change, leaving the existing predicates for a followup PR.

I've also regenerated the fixtures and rebased.

@shonfeder
shonfeder requested review from VojtechStep and gares August 9, 2026 17:26
@gares
gares merged commit a999f29 into LPCIC:master Aug 10, 2026
10 checks passed
@shonfeder
shonfeder deleted the sys-builtins branch August 10, 2026 13:39
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.

3 participants