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

Forbid #require in dune files in OCaml syntax #938

Merged
3 commits merged into from Jul 2, 2018
Merged

Forbid #require in dune files in OCaml syntax #938

3 commits merged into from Jul 2, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jul 1, 2018

#require was restricted to #require "unix" a while ago as allowing files in OCaml syntax to load anything didn't seem right. We initially kept "unix" as some projects were using it, but we can now forbid #require entirely in dune files.

Forbidding unix improves portability.

@ghost ghost requested a review from rgrinberg July 1, 2018 21:35
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost
Copy link
Author

ghost commented Jul 1, 2018

I looked at the projects using #require "unix" in the dune-universe. It's mostly used for reading the output of a command via Unix.open_process_in .... It seems a bit painful to do without unix, so I added a function run_and_read_lines to the plugin API.

While we don't want to encourage the use of the OCaml syntax, we don't support enough yet to replace the current uses cases for it, so it doesn't seem worth making it difficult for users to switch from jbuilder to dune just because of not allowing unix anymore.

let rec loop ic acc =
match input_line ic with
| exception End_of_file -> close_in ic; List.rev acc
| line -> loop ic (line :: acc)
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to the Io module? Note that I see so much opportunity for code reuse, but keeping the application code uncluttered is nice.

Copy link
Author

Choose a reason for hiding this comment

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

It's not a trivial move since this is for plugins

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, that's right. Bit of a shame.

let rec loop ic acc =
match input_line ic with
| exception End_of_file -> close_in ic; List.rev acc
| line -> loop ic (line :: acc)
Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, that's right. Bit of a shame.

@ghost ghost merged commit 438fef9 into ocaml:master Jul 2, 2018
This pull request was closed.
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.

2 participants