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.

Sign up for GitHub

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

Jump to bottom

Compile flags #3679

Closed
wants to merge 8 commits into from
Closed

Compile flags #3679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
be1c448
sort merlin lines after sanitizing
Jul 16, 2020
e4fb2a8
Merge branch 'master' into master
lubegasimon Jul 16, 2020
a066c48
avoiding mutation during sanitazation and sorting of merlin file
lubegasimon Jul 20, 2020
ab0e7e2
Merge branch 'master' into master
lubegasimon Jul 20, 2020
ddb5b1e
avoid mutation and sort merlin lines after sanitizing
lubegasimon Jul 22, 2020
403dd19
Merge remote-tracking branch 'upstream/master'
lubegasimon Jul 30, 2020
897a8fa
Merge remote-tracking branch 'upstream/master'
lubegasimon Aug 2, 2020
5e204ce
#766:enable specifying flags when compiling binaries from inline tests
lubegasimon Aug 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/dune/exe_rules.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ let executables_rules ~sctx ~dir ~expander ~dir_contents ~scope ~compile_info
else
l
in
let flags = SC.ocaml_flags sctx ~dir exes.buildable in
let flags = SC.ocaml_flags sctx ~dir exes.buildable.flags in
let link_deps = Dep_conf_eval.unnamed ~expander exes.link_deps in
let foreign_archives = exes.buildable.foreign_archives |> List.map ~f:snd in
let link_flags =
Expand Down
9 changes: 7 additions & 2 deletions src/dune/inline_tests.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ include Sub_system.Register_end_point (struct
; deps : Dep_conf.t list
; modes : Mode_conf.Set.t
; flags : Ordered_set_lang.Unexpanded.t
; compile_flags : Ordered_set_lang.Unexpanded.t
; backend : (Loc.t * Lib_name.t) option
; libraries : (Loc.t * Lib_name.t) list
}
Expand All @@ -182,6 +183,7 @@ include Sub_system.Register_end_point (struct
(let+ loc = loc
and+ deps = field "deps" (repeat Dep_conf.decode) ~default:[]
and+ flags = Ordered_set_lang.Unexpanded.field "flags"
and+ compile_flags = Ordered_set_lang.Unexpanded.field "compile_flags"
and+ backend = field_o "backend" (located Lib_name.decode)
and+ libraries =
field "libraries" (repeat (located Lib_name.decode)) ~default:[]
Expand All @@ -190,7 +192,7 @@ include Sub_system.Register_end_point (struct
(Dune_lang.Syntax.since syntax (1, 11) >>> Mode_conf.Set.decode)
~default:Mode_conf.Set.default
in
{ loc; deps; flags; backend; libraries; modes })
{ loc; deps; flags; compile_flags; backend; libraries; modes })

(* We don't use this at the moment, but we could implement it for debugging
purposes *)
Expand Down Expand Up @@ -280,7 +282,10 @@ include Sub_system.Register_end_point (struct
Compilation_context.create () ~super_context:sctx ~expander ~scope
~obj_dir ~modules ~opaque:(Explicit false) ~requires_compile:runner_libs
~requires_link:(lazy runner_libs)
~flags:(Ocaml_flags.of_list [ "-w"; "-24"; "-g" ])
~flags:
(Ocaml_flags.append_common
(Super_context.ocaml_flags sctx ~dir lib.buildable.flags)
[ "-w"; "-24"; "-g" ])
~js_of_ocaml:(Some lib.buildable.js_of_ocaml) ~dynlink:false
~package:(Option.map lib.public ~f:Dune_file.Public_lib.package)
in
Expand Down
2 changes: 1 addition & 1 deletion src/dune/lib_rules.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ let cctx (lib : Library.t) ~sctx ~source_modules ~dir ~expander ~scope
else
Required
in
let flags = Super_context.ocaml_flags sctx ~dir lib.buildable in
let flags = Super_context.ocaml_flags sctx ~dir lib.buildable.flags in
let obj_dir = Library.obj_dir ~dir lib in
let vimpl = Virtual_rules.impl sctx ~lib ~scope in
let ctx = Super_context.context sctx in
Expand Down
4 changes: 2 additions & 2 deletions src/dune/super_context.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,10 +295,10 @@ let build_dir_is_vendored build_dir =
in
Option.value ~default:false opt

let ocaml_flags t ~dir (x : Dune_file.Buildable.t) =
let ocaml_flags t ~dir (f : Ocaml_flags.Spec.t) =
let expander = Env_tree.expander t.env_tree ~dir in
let flags =
Ocaml_flags.make ~spec:x.flags
Ocaml_flags.make ~spec:f
~default:(get_node t.env_tree ~dir |> Env_node.ocaml_flags)
~eval:(Expander.expand_and_eval_set expander)
in
Expand Down
2 changes: 1 addition & 1 deletion src/dune/super_context.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ val internal_lib_names : t -> Lib_name.Set.t
(** Compute the ocaml flags based on the directory environment and a buildable
stanza *)
val ocaml_flags :
t -> dir:Path.Build.t -> Dune_file.Buildable.t -> Ocaml_flags.t
t -> dir:Path.Build.t -> Ocaml_flags.Spec.t -> Ocaml_flags.t

val foreign_flags :
t
Expand Down
5 changes: 5 additions & 0 deletions test/blackbox-tests/test-cases/inline_tests/dune-file.t/dune-file-compile-flags/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
(library
(name compile_flags_test)
(inline_tests
(backend foo)
(compile_flags -bar)))
1 change: 1 addition & 0 deletions test/blackbox-tests/test-cases/inline_tests/dune-file.t/dune-file-compile-flags/dune-project
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
(lang dune 2.7)
7 changes: 7 additions & 0 deletions test/blackbox-tests/test-cases/inline_tests/dune-file.t/run.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,10 @@ package:
Entering directory 'dune-file-user'
inline_test_runner_foo_tests alias runtest
414243

We run the inline tests with a program that passes in specifc compile flags:

$ export OCAMLPATH=$PWD/_install/lib; dune runtest --root dune-file-compile-flags
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not %100 sure what the best way to test this will be, but I think something like the following might work:

  • Create a new test file under the inline_tests directory called something like /compiler-flags.t/run.t
  • Set up a backend runner there that just works to detect whether certain conspicuous compiler flags have been set
  • Set up one or two example targets that build the tests using different compiler flags

Do you have any thoughts for a more elegant test set up @jeremiedimino?

Copy link

Choose a reason for hiding this comment

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

That seems about right. You could also pass a flag that you are sure the compiler doesn't accept and observe the compilation failure.

BTW, the above test is using the OCAMLPATH trick to test that inline test backends work even when installed on the system. But since we already tested that, we don't need to use this method again and we can instead use a locally defined backend. So the shell command could just be: dune runtest dune-file-compile-flags.

Entering directory 'dune-file-compile-flags'
inline_test_runner_compile_flags_test alias runtest
414243
1 change: 1 addition & 0 deletions ...lackbox-tests/test-cases/merlin/merlin-tests.t/sanitize-dot-merlin/sanitize_dot_merlin.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
open Printf
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this should be part of the PR.

Copy link
Collaborator Author

@lubegasimon lubegasimon Aug 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

true, it should not be part of it @shonfeder, am not so certain why such git history reflects in here, I have done git reset <commitID>, but all in vain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd either rebase just the relevant changes onto master or create a fresh branch off of master and just cherry pick in the relevant commits from here.

open List

let process_line =
let path_re = Str.regexp {|^\([SB]\) /.+/lib/\(.+\)$|} in
Expand Down