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

Add support for -H #1

Closed
wants to merge 6 commits into from
Closed

Add support for -H #1

wants to merge 6 commits into from

Conversation

nojb
Copy link
Member

@nojb nojb commented May 17, 2024

This PR is not for merging, but to discuss @MA0010's progress on his current task (which, as the title indicates, is to add support for -H flag to Dune, see ocaml/ocaml#12246 and ocaml#9333).

Copy link
Member Author

@nojb nojb left a comment

Choose a reason for hiding this comment

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

I did a quick pass and left some general comments.

bin/main.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/lib.ml Outdated Show resolved Hide resolved
src/dune_rules/lib.ml Outdated Show resolved Hide resolved
src/dune_rules/lib.ml Outdated Show resolved Hide resolved
src/dune_rules/lib.ml Outdated Show resolved Hide resolved
Copy link
Member Author

@nojb nojb left a comment

Choose a reason for hiding this comment

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

I left a few comments below. Also, note that the Dune codebase is formatted using ocamlformat; you should install it in your switch and do make fmt to auto-format your changes. But note that you will have to do that in a 5.2 (or earlier) switch because ocamlformat does not yet support 5.3.

src/dune_rules/lib.ml Outdated Show resolved Hide resolved
src/dune_rules/lib.ml Outdated Show resolved Hide resolved
src/dune_rules/lib_rules.ml Outdated Show resolved Hide resolved
src/dune_rules/coq/coq_rules.ml Outdated Show resolved Hide resolved
src/dune_rules/lib.mli Outdated Show resolved Hide resolved
Copy link
Member Author

@nojb nojb left a comment

Choose a reason for hiding this comment

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

I left a few suggestions for improvements. Also:

  • Apply ocamlformat, this will get rid of several unnecessary whitespace diffs.
  • See if you can make Lib.closure return the same type as before, this will simplify the diff.

src/dune_rules/lib_flags.ml Outdated Show resolved Hide resolved
src/dune_rules/lib_flags.ml Outdated Show resolved Hide resolved
src/dune_rules/lib_flags.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/exe_rules.ml Outdated Show resolved Hide resolved
src/dune_rules/exe_rules.ml Outdated Show resolved Hide resolved
src/ocaml/version.mli Outdated Show resolved Hide resolved
src/dune_rules/module_compilation.ml Outdated Show resolved Hide resolved
src/dune_rules/module_compilation.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.mli Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.mli Outdated Show resolved Hide resolved
src/dune_rules/exe_rules.ml Outdated Show resolved Hide resolved
src/dune_rules/foreign_rules.mli Outdated Show resolved Hide resolved
src/dune_rules/mdx.ml Outdated Show resolved Hide resolved
src/dune_rules/merlin/merlin.ml Outdated Show resolved Hide resolved
src/dune_rules/merlin/merlin.mli Outdated Show resolved Hide resolved
src/dune_rules/module_compilation.ml Outdated Show resolved Hide resolved
src/dune_rules/toplevel.ml Outdated Show resolved Hide resolved
@MA0010 MA0010 force-pushed the hidden_deps branch 2 times, most recently from 44a170e to 58445e1 Compare June 4, 2024 09:49
Copy link
Member Author

@nojb nojb left a comment

Choose a reason for hiding this comment

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

Left some comments. The code of the test can be improved, but I will do that in a follow-up review.

Once the changes of the review have been done, I would like you to experiment with the following change to make the diff cleaner and less invasiave:

Instead of changing the interface of Lib_flags.L.include_flags to take a (Lib.t * bool) list, let's keep Lib.t list (= t) and instead let us pass an optional parameter ?hidden:(Lib.t -> bool) which would be only passed when called from Compilation_context.t. Thanks!

test/blackbox-tests/test-cases/hidden-deps.t/run.t Outdated Show resolved Hide resolved
test/blackbox-tests/test-cases/hidden-deps.t/bash.sh Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/lib_rules.ml Outdated Show resolved Hide resolved
src/dune_rules/merlin/ocaml_index.ml Outdated Show resolved Hide resolved
src/dune_rules/odoc.ml Outdated Show resolved Hide resolved
src/dune_rules/toplevel.ml Outdated Show resolved Hide resolved
src/ocaml/version.mli Outdated Show resolved Hide resolved
;;

type mode =
{ lib_mode : Lib_mode.t
; melange_emit : bool
}

let include_paths =
let add_public_dir ~visible_cmi obj_dir acc mode =
let include_paths_gen =
Copy link
Member Author

Choose a reason for hiding this comment

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

~init, ~add and ~remove should be arguments of include_paths_gen directly.

Comment on lines 177 to 178
let add acc p _ = Path.Set.add acc p in
let remove = Path.Set.remove in
Copy link
Member Author

Choose a reason for hiding this comment

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

Let's inline these two definitions.

@@ -76,3 +76,6 @@ val has_META_files : t -> bool

(** Whether the compiler supports occurrences indexation *)
val supports_bin_annot_occurrences : t -> bool

(**Whether the compiler supports -H flag for including libs*)
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
(**Whether the compiler supports -H flag for including libs*)
(** Whether the compiler supports the -H flag *)

> "No input files")
> dune build
> ;;
> esac
Copy link
Member Author

Choose a reason for hiding this comment

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

What is this test testing?

@nojb
Copy link
Member Author

nojb commented Jun 11, 2024

To be continued upstream.

@nojb nojb closed this Jun 11, 2024
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