Add c_flags and cxx_flags to env profile settings #1700

gretay-js · 2018-12-20T16:32:25Z

Add support for c_flags and cxx_flags fields in profile settings of env stanza. This is in addition to existing ocaml flags in env. It enables us to specify compilation flags per profile when building libraries with C or C++ stubs.

rgrinberg

Thank you for your contribution. This feature is quite useful indeed.

Would you mind adding this feature to CHANGES as well?

src/dune_env.ml

src/env_node.ml

src/super_context.mli

src/lib_rules.ml

test/blackbox-tests/test-cases/env-cflags/dune-project

rgrinberg · 2018-12-20T20:04:25Z

src/super_context.ml

+  end
+
+
+let cxx_flags_gather t ~dir ~(lib : Library.t) ccg =


Why do we need the _gather suffix here?

There is another cxx_flags function in Super_context, and a corresponding field of t. The existing function inspects context.ocamlc_flags and from them computes cxx_flags. Would it be better to rename existing cxx_flags to something like cxx_flags_from_ocamlc_flags or cxx_flags_default?

I think that it would be better to remove the original function. It seems like we shouldn't really be using it anymore as the cxx_flags are now per directory.

The original cxx_flags has been used in combination with expanded lib.cxx_flags in lib_rules.ml to build_cxx_file. The new cxx_flags_gather replaces lib.cxx_flags. I am not sure why it makes the original cxx_flags obsolete. For that matter, I don't know why the original cxx_flags were computed in this way from ocamlc_cflags and what guarantees that C++ compiler knows about all possible ocamlc_cflags (except -std) used in a project. There must be some assumptions about the use of these cxx_flags that I am missing.

In any case, I attempted to make this change in two steps:

change Super_context to rename the original cxx_flags to cxx_flags_orig and renamed cxx_flags_gather to cxx_flags.
55a4d22

remove cxx_flags_orig from Super_context.
They are no longer used anywhere except in Pform.create, where they are getting an akward special treatment. I replaced it by recomputing them (as originally in super_context) and it is analogous to the way cflags are obtained in Pform.
61045a5

My main question is whether cxx_flags_orig need to be conjoined to the new cxx_flags or simply dropped from build_cxx_files in lib_rule. For now, I dropped them, but it might not be safe: removing the original cxx_flags might break other things in dune or external projects that rely on these flags being inferred from ocamlc_cflags.

There is a test in the testsuite for it that passes. I haven't had a chance to test it in a project that actually compiles, because the stubs in my project are in C, but I can create such a test, if this solution seems good to you.

For that matter, I don't know why the original cxx_flags were computed in this way from ocamlc_flags and what guarantees that C++ compiler knows about all possible ocamlc_flags (except -std) used in a project. The must be some assumptions about the use of these cxx_flags that I am missing.

In older version of OCaml there were no *_cppflags in ocamlc -config. We had to take the defaults from somewhere.

My main question is whether cxx_flags_orig need to be conjoined to the new cxx_flags or simply dropped from build_cxx_files in lib_rule. For now, I dropped them, but it might not be safe: removing the original cxx_flags might break other things in dune or external projects that rely on these flags being inferred from ocamlc_flags.

Those are the defaults so they should be inherited. If a user wants to drop them, then setting them explicitly is always possible (without the use of :standard).

Your changes look good to me. Just to make sure we agree on how things should look like. This is the desired behavior of flags:

The default set of flags is inherited from $ ocamlc -config. This detection should be done in a way that understands the differences between different version of OCaml. Things aren't done correctly now, but we can try fixing them in subsequent PR's.

The user may set flags in the environment. This can be done by appending to the set of flags with (:standard -foo bar) or simply resetting it with (-foo bar).

Per library flags may also be set in the stanza itself. These follow the same rules as 2).

Doing (run %{cxx} foo.cxx) should use the flags defined in the environment.

It seems like that the current code does all of this correctly except 4).

There is a test in the testsuite for it that passes. I haven't had a chance to test it in a project that actually compiles, because the stubs in my project are in C, but I can create such a test, if this solution seems good to you.

Tests would indeed be useful. Especially for point 4).

Thank you for the explanations! It makes sense. I'll try to add a test for (4).

I added a test for (4) as far as I understand it, but the test fails because I do not know how to implement (4). Please can you tell me if the test captures what you meant, and if so how to fix the code.

My main question is whether cxx_flags_orig need to be conjoined to the new cxx_flags or simply dropped from build_cxx_files in lib_rule. For now, I dropped them, but it might not be safe: removing the original cxx_flags might break other things in dune or external projects that rely on these flags being inferred from ocamlc_cflags.

Those are the defaults so they should be inherited. If a user wants to drop them, then setting them explicitly is always possible (without the use of :standard).

It looks like cxx_flags_orig was always added in lib_rules, regardless of ":standard".
After removing this default in commit 61045a5, I have noticed that one package from opam fails to build with this version.
The package is re2 and the problem is in
https://github.com/janestreet/re2/blob/master/src/jbuild.
Adding -fPIC at least on my machine fixes the problem, not sure if that's a good solution in general.

It looks like cxx_flags_orig was always added in lib_rules, regardless of ":standard".
After removing this default in commit 61045a5, I have noticed that one package from opam fails to build with this version.
The package is re2 and the problem is in
https://github.com/janestreet/re2/blob/master/src/jbuild.
Adding -fPIC at least on my machine fixes the problem, not sure if that's a good solution in general.

This is now fixed. Thanks to @xclerc for help!

src/env_node.mli

rgrinberg · 2018-12-20T20:07:02Z

Please sign your commits according to: https://github.com/ocaml/dune/blob/master/CONTRIBUTING.md

rgrinberg · 2019-01-11T16:31:43Z

I reviewed the latest commits, it looks fine to me. Could you rebase your PR?

I added a test for (4) as far as I understand it, but the test fails because I do not know how to implement (4). Please can you tell me if the test captures what you meant, and if so how to fix the code.

I'm a bit confused here. As far as I can tell, your test shows the desired behavior is exhibited and the flags from the env are being passed to (run %{cxx}). However, I don't see any code that passes these flags to %{cxx}. A way to implement this would be to update the bindings for the expander in that directory btw.

Btw, I just realized that we haven't accounted for configurator here. Configurator also compiles C/C++ files so perhaps it should also inherit the environment flags. Currently, it accesses them straight from the context. Do you think it would be desirable to make configurator aware of the setting in env?

Btw: some commits are missing the DCO. Once you've done rebasing, I believe that $ git rebase master --signoff should restore the signatures to all commits for you.

test/blackbox-tests/test-cases/env-cflags/run.t

rgrinberg · 2019-01-17T16:43:31Z

Also, let's see what @diml has to say about having configurator respect these c flags:

Btw, I just realized that we haven't accounted for configurator here. Configurator also compiles C/C++ files so perhaps it should also inherit the environment flags. Currently, it accesses them straight from the context. Do you think it would be desirable to make configurator aware of the setting in env?

ghost · 2019-01-17T17:13:43Z

I'm not entirely sure. I suppose both choices make sense. We can probably start with the option that's the simplest to implement and see how it goes.

avsm · 2019-01-17T17:58:50Z

I think the main usecase for this feature will be to add:

static analysis cflags like new warnings
optimisation levels
include directories to os-specific locations (/opt/foo/include) for a particular build

In all of these cases, it seems more useful for configurator to respect the passed in environment flags. It's just important that these cflags are passed consistently to all the dune-related things that use them. It would be most useful if configurator could record somewhere in the build tree the actual cflags that it ended up using (i.e. the combination of the env and local cflags)

ghost · 2019-01-17T18:22:57Z

Ok, that makes sense. However, let's not make it a requirement to merge this PR. It can be done later in a subsequent PR.

rgrinberg · 2019-01-17T18:24:40Z

Indeed, one of us can implement this. I've created #1771 to track this.

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

and fix a typo. Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

gretay-js · 2019-01-21T09:31:56Z

I rebased and fixed expected test outputs to check the defaults based on ocamlc -config using some horrendous combination of sed commands. There must be a better way but this works for now and there are other uses of sed in the test suite.

I'm a bit confused here. As far as I can tell, your test shows the desired behavior is exhibited and the flags from the env are being passed to (run %{cxx}). However, I don't see any code that passes these flags to %{cxx}.

The test fails and I don't know how to fix the code.

A way to implement this would be to update the bindings for the expander in that directory btw.

Can you please give me a bit more direction for it?

rgrinberg · 2019-01-28T19:21:49Z

In pform.ml, %{cc} and %{cxx} are defined accordingly:

      ; "cc"             , strings (context.c_compiler :: cflags)
      ; "cxx"            , strings (context.c_compiler :: cxx_flags)

But that's no longer sufficient and we should update the expander for every Env_node.t. It's possible to do this by updating the expander with the following set of functions:

Expander.add_bindings
Pform.Map.of_list_exn

Where the flags will be fetched from the current env_node (using the c_flags and cxx_flags functions you've defined) all inside the Env.expander function.

rgrinberg · 2019-01-28T19:45:24Z

test/blackbox-tests/test-cases/env-cflags/run.t

@@ -0,0 +1,18 @@
+  $ export STANDARD_C_FLAGS=`ocamlc -config | grep ocamlc_cflags | sed 's/ocamlc_cflags: //'`
+  $ export STANDARD_CXX_FLAGS=`ocamlc -config | grep ocamlc_cflags | sed 's/ocamlc_cflags: //' | sed 's/-std=[^ ]* //' | sed 's/-std=[^$]*//'`


I think CI shows that this doesn't work. Dune starts a new sh process for every $ so the environment variables aren't saved. I'm not sure how to workaround this except for writing your own .sh files that will wrap the command with these substitutions.

rgrinberg · 2019-02-12T10:14:07Z

@gretay-js are you still planning on finishing this? Otherwise I can take over. This feature would be really nice to have.

gretay-js · 2019-02-12T12:04:44Z

Sorry about the delay. Let me have another go at fixing it.

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

gretay-js · 2019-02-15T16:17:53Z

In commit a98b830, I fixed the test failures that depend on environment variables as you suggested by adding a script that filters output of printenv.

Regarding bindings for cc and cxx variables that depend on the bindings of c_flags and cxx_flags variables. Commit 675a0cd attempts to deal with it following your comment earlier, but it doesn't work and doesn't even build. The problem is that Env.cxx_flags functions that I added return (unit, string list) Build.t and add_bindings expects a string list directly.
I am sorry I don't know how to fix it without changes all over the code. Can I leave it to you?

rgrinberg · 2019-02-17T10:54:08Z

Yeah, it's quite a bit more complicated than I expected. The idea is that we will not be expanding cc and cxx in a static context anymore. We need to expand them the way we handle macros such as %{read}. That is, %{cc} and %{cxx} should be able to introduce dependencies.

Unfortunately, that means that it will no longer be possible to expand %{cc} in static contexts. While that's not so bad, it is technically a breaking change.

However, if we're introducing full support for C/C++ dependencies I wonder if it makes sense to have %{cc} and %{cxx} at all. Perhaps we should just consider deprecating these variables? @diml would that be acceptable?

ghost · 2019-02-18T13:07:33Z

We need a replacement first. These variables might be used in contexts that can't be easily changed:

(run ./some_program.exe --c-compiler %{cc} ...)

So it seems better to me if we keep them. It most places, things that are expanded dynamically can also be expanded statically when they don't require dynamic expansion. Can't we do the same here? This way it wouldn't break anything.

rgrinberg · 2019-02-21T13:18:56Z

This PR has been revived in #1860

@aalekseyev

CHANGES: - Clean up watch mode polling loop: improves signal handling and error handling during polling (ocaml/dune#1912, fix ocaml/dune#1907, fix ocaml/dune#1671, @aalekseyev) - Change status messages during polling to be one-line, so that the messages are correctly erased by ^K. (ocaml/dune#1912, @aalekseyev) - Add support for `.cxx` extension for C++ stubs (ocaml/dune#1831, @rgrinberg) - Add `DUNE_WORKSPACE` variable. This variable is equivalent to setting `--workspace` in the command line. (ocaml/dune#1711, fix ocaml/dune#1503, @rgrinberg) - Add `c_flags` and `cxx_flags` to env profile settings (ocaml/dune#1700 and ocaml/dune#1800, @gretay-js) - Format `dune printenv` output (ocaml/dune#1867, fix ocaml/dune#1862, @emillon) - Add the `(promote-into <dir>)` and `(promote-until-clean-into <dir>)` modes for `(rule ...)` stanzas, so that files can be promoted in another directory than the current one. For instance, this is used in merlin to promote menhir generated files in a directory that depends on the version of the compiler (ocaml/dune#1890, @diml) - Improve error message when `dune subst` fails (ocaml/dune#1898, fix ocaml/dune#1897, @rgrinberg) - Add more GC counters to catapult traces (fix908, @rgrinberg) - Add a preprocessor shim for the `let+` syntax of OCaml 4.08 (ocaml/dune#1899, implements ocaml/dune#1891, @diml) - Fix generation of `.merlin` files on Windows. `\` characters needed to be escaped (ocaml/dune#1869, @mlasson) - Fix 0 error code when `$ dune format-dune-file` fails. (ocaml/dune#1915, fix ocaml/dune#1914, @rgrinberg) - Configurator: deprecated `query_expr` and introduced `query_expr_err` which is the same but with a better error in case it fails. (ocaml/dune#1886, @ejgallego)

@aalekseyev

CHANGES: - Clean up watch mode polling loop: improves signal handling and error handling during polling (ocaml/dune#1912, fix ocaml/dune#1907, fix ocaml/dune#1671, @aalekseyev) - Change status messages during polling to be one-line, so that the messages are correctly erased by ^K. (ocaml/dune#1912, @aalekseyev) - Add support for `.cxx` extension for C++ stubs (ocaml/dune#1831, @rgrinberg) - Add `DUNE_WORKSPACE` variable. This variable is equivalent to setting `--workspace` in the command line. (ocaml/dune#1711, fix ocaml/dune#1503, @rgrinberg) - Add `c_flags` and `cxx_flags` to env profile settings (ocaml/dune#1700 and ocaml/dune#1800, @gretay-js) - Format `dune printenv` output (ocaml/dune#1867, fix ocaml/dune#1862, @emillon) - Add the `(promote-into <dir>)` and `(promote-until-clean-into <dir>)` modes for `(rule ...)` stanzas, so that files can be promoted in another directory than the current one. For instance, this is used in merlin to promote menhir generated files in a directory that depends on the version of the compiler (ocaml/dune#1890, @diml) - Improve error message when `dune subst` fails (ocaml/dune#1898, fix ocaml/dune#1897, @rgrinberg) - Add more GC counters to catapult traces (fix908, @rgrinberg) - Add a preprocessor shim for the `let+` syntax of OCaml 4.08 (ocaml/dune#1899, implements ocaml/dune#1891, @diml) - Fix generation of `.merlin` files on Windows. `\` characters needed to be escaped (ocaml/dune#1869, @mlasson) - Fix 0 error code when `$ dune format-dune-file` fails. (ocaml/dune#1915, fix ocaml/dune#1914, @rgrinberg) - Configurator: deprecated `query_expr` and introduced `query_expr_err` which is the same but with a better error in case it fails. (ocaml/dune#1886, @ejgallego) - Make sure `(menhir (mode promote) ...)` stanzas are ignored when using `--ignore-promoted-rules` or `-p` (ocaml/dune#1917, @diml)

gretay-js requested review from emillon and rgrinberg as code owners December 20, 2018 16:32

rgrinberg requested changes Dec 20, 2018

View reviewed changes

gretay-js force-pushed the env_c_flags branch from 61ed09e to c9acd0b Compare December 21, 2018 11:07

rgrinberg reviewed Jan 11, 2019

View reviewed changes

test/blackbox-tests/test-cases/env-cflags/run.t Outdated Show resolved Hide resolved

rgrinberg mentioned this pull request Jan 17, 2019

Configurator should support c_flags and cxx_flags #1771

Open

rgrinberg added this to the 1.7.0 milestone Jan 17, 2019

gretay-js force-pushed the env_c_flags branch from 479a6f5 to a8056c0 Compare January 21, 2019 09:21

gretay-js added 11 commits January 21, 2019 09:23

Add c_flags to env profile settings

c0eafdb

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

update documentation of env

23f2ac7

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

Add tests for c_flags in env

05a046f

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

fix PR review comments

f571836

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

rename cxx_flags in super_context

e264566

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

remove cxx_flags_orig from super_context

a16b9db

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

Remove ccg argument from c_flag and cxx_flags of supercontext.

131a73d

and fix a typo. Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

Add test for run action with env flags (fails)

0cd36d3

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

Fix cxx_flags default

82008bc

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

fix test

67f9820

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

Updated testsuite expected outputs

9df8f00

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

gretay-js force-pushed the env_c_flags branch from a8056c0 to 9df8f00 Compare January 21, 2019 09:24

rgrinberg reviewed Jan 28, 2019

View reviewed changes

gretay-js added 2 commits February 15, 2019 16:10

Fix test script that depend on printenv

a98b830

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

NOT WORKING: cc and cxx variables should depend on c_flags and cxx_flags

675a0cd

Signed-off-by: Greta Yorsh <gyorsh@janestreet.com>

rgrinberg modified the milestones: 1.7.0, 1.8.0 Feb 17, 2019

rgrinberg closed this Feb 21, 2019

rgrinberg mentioned this pull request Mar 7, 2019

[new release] dune (1.8.0) ocaml/opam-repository#13603

Closed

rgrinberg mentioned this pull request Mar 7, 2019

[new release] dune (1.8.0) ocaml/opam-repository#13605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add c_flags and cxx_flags to env profile settings #1700

Add c_flags and cxx_flags to env profile settings #1700

gretay-js commented Dec 20, 2018

rgrinberg left a comment

rgrinberg Dec 20, 2018

gretay-js Dec 21, 2018

rgrinberg Dec 21, 2018

gretay-js Dec 21, 2018 •

edited

Loading

rgrinberg Dec 21, 2018

gretay-js Dec 21, 2018 •

edited

Loading

gretay-js Jan 11, 2019 •

edited

Loading

gretay-js Jan 11, 2019 •

edited

Loading

gretay-js Jan 11, 2019

rgrinberg commented Dec 20, 2018

rgrinberg commented Jan 11, 2019

rgrinberg commented Jan 17, 2019

ghost commented Jan 17, 2019

avsm commented Jan 17, 2019

ghost commented Jan 17, 2019

rgrinberg commented Jan 17, 2019

gretay-js commented Jan 21, 2019 •

edited

Loading

rgrinberg commented Jan 28, 2019

rgrinberg Jan 28, 2019

rgrinberg commented Feb 12, 2019

gretay-js commented Feb 12, 2019

gretay-js commented Feb 15, 2019

rgrinberg commented Feb 17, 2019

ghost commented Feb 18, 2019

rgrinberg commented Feb 21, 2019

		@@ -0,0 +1,18 @@
		$ export STANDARD_C_FLAGS=`ocamlc -config \| grep ocamlc_cflags \| sed 's/ocamlc_cflags: //'`
		$ export STANDARD_CXX_FLAGS=`ocamlc -config \| grep ocamlc_cflags \| sed 's/ocamlc_cflags: //' \| sed 's/-std=[^ ]* //' \| sed 's/-std=[^$]*//'`

Add c_flags and cxx_flags to env profile settings #1700

Add c_flags and cxx_flags to env profile settings #1700

Conversation

gretay-js commented Dec 20, 2018

rgrinberg left a comment

Choose a reason for hiding this comment

rgrinberg Dec 20, 2018

Choose a reason for hiding this comment

gretay-js Dec 21, 2018

Choose a reason for hiding this comment

rgrinberg Dec 21, 2018

Choose a reason for hiding this comment

gretay-js Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

rgrinberg Dec 21, 2018

Choose a reason for hiding this comment

gretay-js Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

gretay-js Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

gretay-js Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

gretay-js Jan 11, 2019

Choose a reason for hiding this comment

rgrinberg commented Dec 20, 2018

rgrinberg commented Jan 11, 2019

rgrinberg commented Jan 17, 2019

ghost commented Jan 17, 2019

avsm commented Jan 17, 2019

ghost commented Jan 17, 2019

rgrinberg commented Jan 17, 2019

gretay-js commented Jan 21, 2019 • edited Loading

rgrinberg commented Jan 28, 2019

rgrinberg Jan 28, 2019

Choose a reason for hiding this comment

rgrinberg commented Feb 12, 2019

gretay-js commented Feb 12, 2019

gretay-js commented Feb 15, 2019

rgrinberg commented Feb 17, 2019

ghost commented Feb 18, 2019

rgrinberg commented Feb 21, 2019

gretay-js Dec 21, 2018 •

edited

Loading

gretay-js Dec 21, 2018 •

edited

Loading

gretay-js Jan 11, 2019 •

edited

Loading

gretay-js Jan 11, 2019 •

edited

Loading

gretay-js commented Jan 21, 2019 •

edited

Loading