OSL handling for {c,cxx}_names #1788

rgrinberg · 2019-01-29T13:34:19Z

This PR changes the handling for {c,cxx}_names to be more like the module fields. In particular, there's no need to specify paths - objects files will do. Additionally, this adds some proper handling to edge cases where c/cpp sources are overlapping.

ghost · 2019-01-29T13:52:24Z

That seems good to me.

ghost · 2019-01-29T13:52:59Z

test/blackbox-tests/test-cases/menhir/run.t

@@ -75,8 +75,8 @@ Reproduction case for #1781, only the .ml and .mli should be promoted:
  $ dune build @all --root promote
  Entering directory 'promote'
  $ ls -1 promote/_build/default | grep mock


maybe we need a LANG=C or something here. Actually, maybe we should set LANG=C in the cram test executable

I pushed a commit that does this but it doesn't seem to have an effect.

It must have been the ordering on my system that was non-C. I just tried your branch and the test are passing on my machine.

Travis still seems to prefer the old order. Shall we just sort the results explicitly?

Yh, I checked and we are already doing | sort explicitly in a few other tests

ghost · 2019-01-29T13:53:58Z

Maybe we should also test the case where the same name is used by two different libraries?

rgrinberg · 2019-01-30T07:01:29Z

I started with experimenting with making {c,cxx}_names and it's a bit of a mixed bag. The work is still in progress but I pushed a couple of commits that sketch out the general idea.

A positive is that it allows us to improve error handling and easily fix some long standing issues like support for multiple file extensions. Unfortunately, there is a complexity cost and a bit of asymmetry with how modules are handled in multi dirs. Currently, {c,cxx}_names allows for relative paths to be included unlike the modules field.

What is the use case for specifying relative paths in {c,cxx}_names? And why can't we rely on include_subdirs for the path support. Obviously, it's an issue to change the current behavior, but for the c_executables stanza I'm wondering if we should rethink this behavior.

ghost · 2019-01-30T08:25:34Z

I don't think this was intentional. It was probably implemented this way as we didn't have good support for C names when include_subdirs was added

rgrinberg · 2019-01-30T17:15:07Z

Okay, so I think a good course of action will be to deprecate the current behavior of allowing foo/stub in favor of just using stub and include_subdirs. I will introduce this warning and we can consider eliminating it eventually.

rgrinberg · 2019-01-31T05:38:32Z

Okay, this is ready for review. I still need to

Create & check the reverse map to have a good error message when a C/Cxx is used twice in different stanzas.
Verify that the directory i'm using to compile the stubs is correct.

rgrinberg · 2019-01-31T05:41:00Z

src/lib_rules.ml

    SC.add_rule sctx ~loc ~dir
      (Expander.expand_and_eval_set expander lib.c_flags
         ~standard:(Build.return (Context.cc_g ctx))
       >>>
       Build.run
         (* We have to execute the rule in the library directory as
            the .o is produced in the current directory *)
-         ~dir:(Path.parent_exn src)


My fix doesn't seem entirely right to me but what we had before was suspicious as well. The parent of the source dir of the file isn't necessarily the library directory.

rgrinberg · 2019-01-31T05:41:44Z

test/blackbox-tests/test-cases/duplicate-c-cxx/run.t

+c_names with overlapping names in different stanzas
+  $ dune build --root diff-stanza @all
+  Entering directory 'diff-stanza'
+  Multiple rules generated for _build/default/foo$ext_obj:


To fix this I will need to maintain a reverse map from stanzas to sources (like we do for modules)

rgrinberg · 2019-01-31T16:12:03Z

Okay, the sorting thing is still somehow an issue:

  $ ls -1 promote/_build/default | sort | grep mock
-  parser__mock.ml.mock
   parser__mock.mli.inferred
+  parser__mock.ml.mock

No idea how this is still an issue.

ghost · 2019-01-31T16:14:55Z

:/ I suggest that we just stuff our own sorting binary in the PATH when executing tests...

src/c.mli

src/c_sources.ml

rgrinberg · 2019-01-31T19:57:06Z

cc @ejgallego This PR bring C/C++ sources to a first class level similar to OCaml modules. It might prove to be a useful reference if you plan to add similar to Coq sources.

rgrinberg · 2019-01-31T23:28:20Z

And now there's proper handling when a stub is reused between stanzas.

emillon · 2019-02-01T17:00:54Z

For sort, setting LC_ALL=C might also help.

rgrinberg · 2019-02-01T21:57:02Z

WIP:

This pr accidentally moves the location of .o files in (include_subdirs unqualified). It puts them into the lib directory rather a dedicated object directory, @diml how come we don't put .o files in the respective obj dir?

ghost · 2019-02-04T11:14:08Z

Last time I checked, it was hard to put these files in a directory different from the one where the file is compiled from. For instance, that's the reason why we originally called cd src & ocamlc -c file.c rather than ocamlc -c src/file.c. The latter produces better error message, however it produces file.o rather than src/file.o. It's possible that this was an issue with an older version of OCaml. Do I understand correctly that you are doing something similar in this PR and it seems to work?

rgrinberg · 2019-02-04T16:33:43Z

Indeed it seems to work. But actually, I'd like to do more and move the obj files to the .obj dir. I'll experiment with that in a later pr but make sure that we have here works.

rgrinberg · 2019-02-04T18:03:46Z

This PR now disallows duplicate object files in the same library.

emillon · 2019-02-04T20:05:13Z

src/dune_file.ml

@@ -838,8 +823,8 @@ module Library = struct
         field "ppx_runtime_libraries" (list (located Lib_name.decode)) ~default:[]
       and c_flags = field_oslu "c_flags"
       and cxx_flags = field_oslu "cxx_flags"
-       and c_names = field "c_names" (list c_name) ~default:[]
-       and cxx_names = field "cxx_names" (list cxx_name) ~default:[]
+       and c_names = field_o "c_names" Ordered_set_lang.decode


Do we need to put that behind a language version check?

I think this change is backwards compatible.

I suppose that if we are using (lang dune <1.7), then we should make sure the arguments are plain lists.

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg · 2019-02-04T21:33:13Z

I misclicked & accidentally merged this PR. @diml @emillon please feel free to continue reviewing this. If this causes a regression, I will revert the PR.

ghost · 2019-02-05T08:17:06Z

src/c_sources.ml

+            | Some source -> (loc, source)
+            | None ->
+              Errors.fail loc "%s does not exist as a C source. \
+                               One of %s must be present"


Nitpicking, but when there is a single element in C.Kind.possible_fns kind s, the error message won't be optimal:

file does not exist as a C source. One of file.c must be present

it would read nicer as:

file does not exist as a C source. file.c must be present

ghost · 2019-02-05T08:18:57Z

src/c_sources.ml

+        let c = eval C.Kind.C c_sources.c c_name (names lib.c_names) in
+        let cxx = eval C.Kind.Cxx c_sources.cxx cxx_name (names lib.cxx_names) in
+        let all = String.Map.union c cxx ~f:(fun _ (_loc1, c) (loc2, cxx) ->
+          Errors.fail loc2 "%a source file is invalid because %a exists"


Error message suggestion:

file.c and file.cpp have conflicting names. You must rename one of them.

ghost · 2019-02-05T08:23:15Z

src/c_sources.mli

+val load_sources
+  :  dir:Path.t
+  -> files:String.Set.t
+  -> C.Source.t String.Map.t C.Kind.Dict.t


I think these functions deserve a comment to explain what the String.Map.t are

ghost · 2019-02-05T08:27:28Z

src/dir_contents.ml

+                                    Path.relative (File_tree.Dir.path ft_dir)
+                                      "_unknown_"
+                                  | Some d -> File_tree.Dune_file.path d))
+                    "%a file %s appears in several directories:\


It feels like there is something missing in this error message. I suggest to append:

This is not allowed, please rename one of them.

ghost · 2019-02-05T08:34:38Z

Ok. I just made a couple of suggestions for the error messages, but otherwise it looks good to me.

@diml

CHANGES: - Second step of the deprecation of jbuilder: the `jbuilder` binary now emits a warning on every startup and both `jbuilder` and `dune` emit warnings when encountering `jbuild` files (ocaml/dune#1752, @diml) - Change the layout of build artifacts inside _build. The new layout enables optimizations that depend on the presence of `.cmx` files of private modules (ocaml/dune#1676, @bobot) - Fix merlin handling of private module visibility (ocaml/dune#1653 @bobot) - unstable-fmt: use boxes to wrap some lists (ocaml/dune#1608, fix ocaml/dune#1153, @emillon, thanks to @rgrinberg) - skip directories when looking up programs in the PATH (ocaml/dune#1628, fixes ocaml/dune#1616, @diml) - Use `lsof` on macOS to implement `--stats` (ocaml/dune#1636, fixes ocaml/dune#1634, @xclerc) - Generate `dune-package` files for every package. These files are installed and read instead of `META` files whenever they are available (ocaml/dune#1329, @rgrinberg) - Fix preprocessing for libraries with `(include_subdirs ..)` (ocaml/dune#1624, fix ocaml/dune#1626, @nojb, @rgrinberg) - Do not generate targets for archive that don't match the `modes` field. (ocaml/dune#1632, fix ocaml/dune#1617, @rgrinberg) - When executing actions, open files lazily and close them as soon as possible in order to reduce the maximum number of file descriptors opened by Dune (ocaml/dune#1635, ocaml/dune#1643, fixes ocaml/dune#1633, @jonludlam, @rgrinberg, @diml) - Reimplement the core of Dune using a new generic memoization system (ocaml/dune#1489, @rudihorn, @diml) - Replace the broken cycle detection algorithm by a state of the art one from [this paper](https://doi.org/10.1145/2756553) (ocaml/dune#1489, @rudihorn) - Get the correct environment node for multi project workspaces (ocaml/dune#1648, @rgrinberg) - Add `dune compute` to call internal memoized functions (ocaml/dune#1528, @rudihorn, @diml) - Add `--trace-file` option to trace dune internals (ocaml/dune#1639, fix ocaml/dune#1180, @emillon) - Add `--no-print-directory` (borrowed from GNU make) to suppress `Entering directory` messages. (ocaml/dune#1668, @dra27) - Remove `--stats` and track fd usage in `--trace-file` (ocaml/dune#1667, @emillon) - Add virtual libraries feature and enable it by default (ocaml/dune#1430 fixes ocaml/dune#921, @rgrinberg) - Fix handling of Control+C in watch mode (ocaml/dune#1678, fixes ocaml/dune#1671, @diml) - Look for jsoo runtime in the same dir as the `js_of_ocaml` binary when the ocamlfind package is not available (ocaml/dune#1467, @nojb) - Make the `seq` package available for OCaml >= 4.07 (ocaml/dune#1714, @rgrinberg) - Add locations to error messages where a rule fails to generate targets and rules that require files outside the build/source directory. (ocaml/dune#1708, fixes ocaml/dune#848, @rgrinberg) - Let `Configurator` handle `sizeof` (in addition to negative numbers). (ocaml/dune#1726, fixes ocaml/dune#1723, @Chris00) - Fix an issue causing menhir generated parsers to fail to build in some cases. The fix is to systematically use `-short-paths` when calling `ocamlc -i` (ocaml/dune#1743, fix ocaml/dune#1504, @diml) - Never raise when printing located errors. The code that would print the location excerpts was prone to raising. (ocaml/dune#1744, fix ocaml/dune#1736, @rgrinberg) - Add a `dune upgrade` command for upgrading jbuilder projects to Dune (ocaml/dune#1749, @diml) - When automatically creating a `dune-project` file, insert the detected name in it (ocaml/dune#1749, @diml) - Add `(implicit_transitive_deps <bool>)` mode to dune projects. When this mode is turned off, transitive dependencies are not accessible. Only listed dependencies are directly accessible. (ocaml/dune#1734, ocaml/dune#430, @rgrinberg, @hnrgrgr) - Add `toplevel` stanza. This stanza is used to define toplevels with libraries already preloaded. (ocaml/dune#1713, @rgrinberg) - Generate `.merlin` files that account for normal preprocessors defined using a subset of the `action` language. (ocaml/dune#1768, @rgrinberg) - Emit `(orig_src_dir <path>)` metadata in `dune-package` for dune packages built with `--store-orig-source-dir` command line flag (also controlled by `DUNE_STORE_ORIG_SOURCE_DIR` env variable). This is later used to generate `.merlin` with `S`-directives pointed to original source locations and thus allowing merlin to see those. (ocaml/dune#1750, @andreypopp) - Improve the behavior of `dune promote` when the files to be promoted have been deleted. (ocaml/dune#1775, fixes ocaml/dune#1772, @diml) - unstable-fmt: preserve comments (ocaml/dune#1766, @emillon) - Pass flags correctly when using `staged_pps` (ocaml/dune#1779, fixes ocaml/dune#1774, @diml) - Fix an issue with the use of `(mode promote)` in the menhir stanza. It was previously causing intermediate *mock* files to be promoted (ocaml/dune#1783, fixes ocaml/dune#1781, @diml) - unstable-fmt: ignore files using OCaml syntax (ocaml/dune#1784, @emillon) - Configurator: Add `which` function to replace the `which` command line utility in a cross platform way. (ocaml/dune#1773, fixes ocaml/dune#1705, @Chris00) - Make configurator append paths to `$PKG_CONFIG_PATH` on macOS. Previously it was prepending paths and thus `$PKG_CONFIG_PATH` set by users could have been overridden by homebrew installed libraries (ocaml/dune#1785, @andreypopp) - Disallow c/cxx sources that share an object file in the same stubs archive. This means that `foo.c` and `foo.cpp` can no longer exist in the same library. (ocaml/dune#1788, @rgrinberg) - Forbid use of `%{targets}` (or `${@}` in jbuild files) inside preprocessing actions (ocaml/dune#1812, fixes ocaml/dune#1811, @diml) - Add `DUNE_PROFILE` environment variable to easily set the profile. (ocaml/dune#1806, @rgrinberg) - Deprecate the undocumented `(no_keep_locs)` field. It was only necessary until virtual libraries were supported (ocaml/dune#1822, fix ocaml/dune#1816, @diml) - Rename `unstable-fmt` to `format-dune-file` and remove its `--inplace` option. (ocaml/dune#1821, @emillon). - Autoformatting: `(using fmt 1.1)` will also format dune files (ocaml/dune#1821, @emillon). - Autoformatting: record dependencies on `.ocamlformat-ignore` files (ocaml/dune#1824, fixes ocaml/dune#1793, @emillon)

rgrinberg requested a review from a user January 29, 2019 13:34

rgrinberg force-pushed the duplicate-c-cxx-names branch from 71ae0ff to 6720d98 Compare January 29, 2019 13:46

ghost reviewed Jan 29, 2019

View reviewed changes

ghost approved these changes Jan 29, 2019

View reviewed changes

rgrinberg force-pushed the duplicate-c-cxx-names branch from b6ab6c6 to b2aa626 Compare January 31, 2019 05:36

rgrinberg requested a review from emillon as a code owner January 31, 2019 05:36

rgrinberg changed the title ~~Add test case for duplicate c_names/cxx_names~~ OSL handling for {c,cxx}_names Jan 31, 2019

rgrinberg commented Jan 31, 2019

View reviewed changes

rgrinberg force-pushed the duplicate-c-cxx-names branch from b2aa626 to d9ddc4d Compare January 31, 2019 15:50

ghost suggested changes Jan 31, 2019

View reviewed changes

src/c.mli Outdated Show resolved Hide resolved

src/c.mli Show resolved Hide resolved

src/c.mli Outdated Show resolved Hide resolved

src/c_sources.ml Outdated Show resolved Hide resolved

rgrinberg force-pushed the duplicate-c-cxx-names branch from 0a326d7 to 34a4835 Compare January 31, 2019 16:48

rgrinberg force-pushed the duplicate-c-cxx-names branch from fa5ada2 to 757f174 Compare January 31, 2019 23:07

rgrinberg force-pushed the duplicate-c-cxx-names branch from 67ae483 to c874aef Compare February 1, 2019 21:43

rgrinberg force-pushed the duplicate-c-cxx-names branch from c874aef to 21337f8 Compare February 4, 2019 17:59

emillon reviewed Feb 4, 2019

View reviewed changes

rgrinberg added 12 commits February 5, 2019 04:21

Add C_sources to Dir_contents

d261067

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Add function to access the C sources of libraries

9d60dda

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Add sources to c stubs tests

17f4430

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Add OSL handling for C stubs

675bce5

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Fix handling of C_sources for include_subdirs

eebe359

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

s/foreign_objects/objects/

57f0a70

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

s/split_fn/split_extension/

3e552b5

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

document possible_fns

eb2dd62

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Fix {c,cxx}_name

2457a84

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Add proper error checking for duplicate stub usage

28dd2ee

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Fix error message for duplicate obj

dfa7625

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Update CHANGES regarding duplicate object files

479807c

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg force-pushed the duplicate-c-cxx-names branch from 91d10c1 to 479807c Compare February 4, 2019 21:21

rgrinberg merged commit 479807c into ocaml:master Feb 4, 2019

ghost reviewed Feb 5, 2019

View reviewed changes

rgrinberg mentioned this pull request Feb 5, 2019

Improve error message & add docs #1810

Merged

rgrinberg mentioned this pull request Feb 12, 2019

[new release] dune (1.7.0) ocaml/opam-repository#13444

Merged

nojb mentioned this pull request Sep 16, 2019

[RFC] Clean up and extend existing support for C/C++ libraries #2644

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSL handling for {c,cxx}_names #1788

OSL handling for {c,cxx}_names #1788

rgrinberg commented Jan 29, 2019 •

edited

Loading

ghost commented Jan 29, 2019

ghost Jan 29, 2019

rgrinberg Jan 29, 2019

ghost Jan 29, 2019

rgrinberg Jan 29, 2019

ghost Jan 29, 2019

ghost commented Jan 29, 2019

rgrinberg commented Jan 30, 2019

ghost commented Jan 30, 2019

rgrinberg commented Jan 30, 2019

rgrinberg commented Jan 31, 2019

rgrinberg Jan 31, 2019

rgrinberg Jan 31, 2019

rgrinberg commented Jan 31, 2019

ghost commented Jan 31, 2019

rgrinberg commented Jan 31, 2019

rgrinberg commented Jan 31, 2019

emillon commented Feb 1, 2019

rgrinberg commented Feb 1, 2019 •

edited by aalekseyev

Loading

ghost commented Feb 4, 2019

rgrinberg commented Feb 4, 2019

rgrinberg commented Feb 4, 2019

emillon Feb 4, 2019

rgrinberg Feb 4, 2019

ghost Feb 5, 2019

rgrinberg commented Feb 4, 2019

ghost Feb 5, 2019

ghost Feb 5, 2019

ghost Feb 5, 2019

ghost Feb 5, 2019

ghost commented Feb 5, 2019

OSL handling for {c,cxx}_names #1788

OSL handling for {c,cxx}_names #1788

Conversation

rgrinberg commented Jan 29, 2019 • edited Loading

ghost commented Jan 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jan 29, 2019

rgrinberg commented Jan 30, 2019

ghost commented Jan 30, 2019

rgrinberg commented Jan 30, 2019

rgrinberg commented Jan 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Jan 31, 2019

ghost commented Jan 31, 2019

rgrinberg commented Jan 31, 2019

rgrinberg commented Jan 31, 2019

emillon commented Feb 1, 2019

rgrinberg commented Feb 1, 2019 • edited by aalekseyev Loading

ghost commented Feb 4, 2019

rgrinberg commented Feb 4, 2019

rgrinberg commented Feb 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Feb 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 5, 2019

rgrinberg commented Jan 29, 2019 •

edited

Loading

rgrinberg commented Feb 1, 2019 •

edited by aalekseyev

Loading