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

Unhardcode extensions #2275

Merged
merged 5 commits into from
Jun 13, 2019
Merged

Unhardcode extensions #2275

merged 5 commits into from
Jun 13, 2019

Conversation

rgrinberg
Copy link
Member

This PR moves all extensions string literals to be defined in some module. In the process, I discover that we missed updating the line directive when we added support for the cxx and cc extensions. I refactored the code so that this is no longer possible.

I added this Dpath module mainly to avoid circular deps, but I think @diml wanted to move all dune related Path stuff to dune itself anyway.

@rgrinberg rgrinberg requested review from a user and aalekseyev June 12, 2019 14:22
aalekseyev
aalekseyev reviewed Jun 12, 2019
View reviewed changes
src/c.ml Show resolved Hide resolved
aalekseyev
aalekseyev reviewed Jun 12, 2019
View reviewed changes
src/c.ml Show resolved Hide resolved
aalekseyev
aalekseyev reviewed Jun 12, 2019
View reviewed changes
src/c.ml Outdated Show resolved Hide resolved
aalekseyev
aalekseyev requested changes Jun 12, 2019
View reviewed changes
Copy link
Collaborator

@aalekseyev aalekseyev left a comment

Choose a reason for hiding this comment

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

Seems good. Left some comments.

@aalekseyev
Copy link
Collaborator

By the way, your commit description says that cxx was ignored before, but I can't see that. It seems that only cc was ignored.

@rgrinberg
Copy link
Member Author

@aalekseyev in the previous code, the line directives were generated like this:

let line_directive ~filename:fn ~line_number =
  let directive =
    match Filename.extension fn with
    | ".c" | ".cpp" | ".h" -> "line"
    | _ -> ""
  in
  sprintf "#%s %d %S\n" directive line_number fn

I believe this ignored both .cc and .cxx

@rgrinberg rgrinberg changed the title Unhardcode whenever possible Unhardcode extensions Jun 13, 2019
@ghost
Copy link

ghost commented Jun 13, 2019

I added this Dpath module mainly to avoid circular deps, but I think @diml wanted to move all dune related Path stuff to dune itself anyway.

Yh, now that we have Local_gen it might be easier to move Path.Build to the dune library. Although it's not very urgent.

BTW, it seems to me that we should merge Path_dune_lang and Dpath. They are both additional path functions for the purpose of dune.

@rgrinberg rgrinberg force-pushed the unhardcode-extensions branch from 263812b to c430ba3 Compare June 13, 2019 09:53
rgrinberg added 2 commits June 13, 2019 17:04
@rgrinberg
Use extension constants instead of hard-coding
f32c79f 
This helps to keep consistent and prevents typos

Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg
Introduce Dpath module
e916942 
This module contains path related functionality specific to dune

Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg rgrinberg force-pushed the unhardcode-extensions branch from 64c7833 to a44ae47 Compare June 13, 2019 10:07
rgrinberg added 2 commits June 13, 2019 17:07
@rgrinberg
Fix generating directives for C++ sources
578d543 
cxx and cc sources were ignored were previously ignored

Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg
Update CHANGES
3e04626 
Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg rgrinberg force-pushed the unhardcode-extensions branch from a44ae47 to 3e04626 Compare June 13, 2019 10:07
@rgrinberg
Move Path_dune_lang into Dpath
d461001 
Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg rgrinberg merged commit 93d9bb7 into ocaml:master Jun 13, 2019
@rgrinberg rgrinberg deleted the unhardcode-extensions branch June 13, 2019 10:19
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jul 18, 2019
@rgrinberg
[new release] dune-build-info and dune (1.11.0)
aa4bf16 
CHANGES:

- Don't select all local implementations in `dune utop`. Instead, let the
  default implementation selection do its job. (ocaml/dune#2327, fixes ocaml/dune#2323, @TheLortex,
  review by @rgrinberg)

- Check that selected implementations (either by variants or default
  implementations) are indeed implementations. (ocaml/dune#2328, @TheLortex, review by
  @rgrinberg)

- Don't reserve the `Ppx` toplevel module name for ppx rewriters (ocaml/dune#2242, @diml)

- Redesign of the library variant feature according to the ocaml/dune#2134 proposal. The
  set of variants is now computed when the virtual library is installed.
  Introducing a new `external_variant` stanza. (ocaml/dune#2169, fixes ocaml/dune#2134, @TheLortex,
  review by @diml)

- Add proper line directives when copying `.cc` and `.cxx` sources (ocaml/dune#2275,
  @rgrinberg)

- Fix error message for missing C++ sources. The `.cc` extension was always
  ignored before. (ocaml/dune#2275, @rgrinberg)

- Add `$ dune init project` subcommand to create project boilerplate according
  to a common template. (ocaml/dune#2185, fixes ocaml/dune#159, @shonfeder)

- Allow to run inline tests in javascript with nodejs (ocaml/dune#2266, @hhugo)

- Build `ppx.exe` as compiling host binary. (ocaml/dune#2286, fixes ocaml/dune#2252, @toots, review
  by @rgrinberg and @diml)

- Add a `cinaps` extension and stanza for better integration with the
  [cinaps tool](https://github.com/janestreet/cinaps) tool (ocaml/dune#2269,
  @diml)

- Allow to embed build info in executables such as version and list
  and version of statically linked libraries (ocaml/dune#2224, @diml)

- Set version in `META` and `dune-package` files to the one read from
  the vcs when no other version is available (ocaml/dune#2224, @diml)

- Add a variable `%{target}` to be used in situations where the context
  requires at most one word, so `%{targets}` can be confusing; stdout
  redirections and "-o" arguments of various tools are the main use
  case; also, introduce a separate field `target` that must be used
  instead of `targets` in those situations.  (ocaml/dune#2341, @aalekseyev)

- Fix dependency graph of wrapped_compat modules. Previously, the dependency on
  the user written entry module was omitted. (ocaml/dune#2305, @rgrinberg)

- Allow to promote executables built with an `executable` stanza
  (ocaml/dune#2379, @diml)

- When instantiating an implementation with a variant, make sure it matches
  virtual library's list of known implementations. (ocaml/dune#2361, fixes ocaml/dune#2322,
  @TheLortex, review by @rgrinberg)

- Add a variable `%{ignoring_promoted_rules}` that is `true` when
  `--ingore-promoted-rules` is passed on the command line and false
  otherwise (ocaml/dune#2382, @diml)

- Fix a bug in `future_syntax` where the characters `@` and `&` were
  not distinguished in the names of binding operators (`let@` was the
  same as `let&`) (ocaml/dune#2376, @aalekseyev, @diml)

- Workspaces with non unique project names are now supported. (ocaml/dune#2377, fix ocaml/dune#2325,
  @rgrinberg)

- Improve opam generation to include the `dune` dependncies with the minimum
  constraint set based on the dune language version specified in the
  `dune-project` file. (2383, @avsm)

- The order of fields in the generated opam file now follows order preferred in
  opam-lib. (@avsm, ocaml/dune#2380)

- Fix coloring of error messages from the compiler (@diml, ocaml/dune#2384)

- Add warning `66` to default set of warnings starting for dune projects with
  language verison >= `1.11` (@rgrinberg, @diml, fixes ocaml/dune#2299)

- Add (dialect ...) stanza
  (@nojb, ocaml/dune#2404)

- Add a `--context` argument to `dune install/uninstall` (@diml, ocaml/dune#2412)

- Do not warn about merlin files pre 1.9. This warning can only be disabled in
  1.9 (ocaml/dune#2421, fixes ocaml/dune#2399, @emillon)
@rgrinberg rgrinberg mentioned this pull request Jul 18, 2019
Closed
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jul 22, 2019
@rgrinberg
[new release] dune-build-info and dune (1.11.0)
7d4dbf7 
CHANGES:

- Don't select all local implementations in `dune utop`. Instead, let the
  default implementation selection do its job. (ocaml/dune#2327, fixes ocaml/dune#2323, @TheLortex,
  review by @rgrinberg)

- Check that selected implementations (either by variants or default
  implementations) are indeed implementations. (ocaml/dune#2328, @TheLortex, review by
  @rgrinberg)

- Don't reserve the `Ppx` toplevel module name for ppx rewriters (ocaml/dune#2242, @diml)

- Redesign of the library variant feature according to the ocaml/dune#2134 proposal. The
  set of variants is now computed when the virtual library is installed.
  Introducing a new `external_variant` stanza. (ocaml/dune#2169, fixes ocaml/dune#2134, @TheLortex,
  review by @diml)

- Add proper line directives when copying `.cc` and `.cxx` sources (ocaml/dune#2275,
  @rgrinberg)

- Fix error message for missing C++ sources. The `.cc` extension was always
  ignored before. (ocaml/dune#2275, @rgrinberg)

- Add `$ dune init project` subcommand to create project boilerplate according
  to a common template. (ocaml/dune#2185, fixes ocaml/dune#159, @shonfeder)

- Allow to run inline tests in javascript with nodejs (ocaml/dune#2266, @hhugo)

- Build `ppx.exe` as compiling host binary. (ocaml/dune#2286, fixes ocaml/dune#2252, @toots, review
  by @rgrinberg and @diml)

- Add a `cinaps` extension and stanza for better integration with the
  [cinaps tool](https://github.com/janestreet/cinaps) tool (ocaml/dune#2269,
  @diml)

- Allow to embed build info in executables such as version and list
  and version of statically linked libraries (ocaml/dune#2224, @diml)

- Set version in `META` and `dune-package` files to the one read from
  the vcs when no other version is available (ocaml/dune#2224, @diml)

- Add a variable `%{target}` to be used in situations where the context
  requires at most one word, so `%{targets}` can be confusing; stdout
  redirections and "-o" arguments of various tools are the main use
  case; also, introduce a separate field `target` that must be used
  instead of `targets` in those situations.  (ocaml/dune#2341, @aalekseyev)

- Fix dependency graph of wrapped_compat modules. Previously, the dependency on
  the user written entry module was omitted. (ocaml/dune#2305, @rgrinberg)

- Allow to promote executables built with an `executable` stanza
  (ocaml/dune#2379, @diml)

- When instantiating an implementation with a variant, make sure it matches
  virtual library's list of known implementations. (ocaml/dune#2361, fixes ocaml/dune#2322,
  @TheLortex, review by @rgrinberg)

- Add a variable `%{ignoring_promoted_rules}` that is `true` when
  `--ingore-promoted-rules` is passed on the command line and false
  otherwise (ocaml/dune#2382, @diml)

- Fix a bug in `future_syntax` where the characters `@` and `&` were
  not distinguished in the names of binding operators (`let@` was the
  same as `let&`) (ocaml/dune#2376, @aalekseyev, @diml)

- Workspaces with non unique project names are now supported. (ocaml/dune#2377, fix ocaml/dune#2325,
  @rgrinberg)

- Improve opam generation to include the `dune` dependncies with the minimum
  constraint set based on the dune language version specified in the
  `dune-project` file. (2383, @avsm)

- The order of fields in the generated opam file now follows order preferred in
  opam-lib. (@avsm, ocaml/dune#2380)

- Fix coloring of error messages from the compiler (@diml, ocaml/dune#2384)

- Add warning `66` to default set of warnings starting for dune projects with
  language verison >= `1.11` (@rgrinberg, @diml, fixes ocaml/dune#2299)

- Add (dialect ...) stanza
  (@nojb, ocaml/dune#2404)

- Add a `--context` argument to `dune install/uninstall` (@diml, ocaml/dune#2412)

- Do not warn about merlin files pre 1.9. This warning can only be disabled in
  1.9 (ocaml/dune#2421, fixes ocaml/dune#2399, @emillon)

- Add a new `inline_tests` field in the env stanza to control inline_tests
  framework with a variable (ocaml/dune#2313, @mlasson, original idea by @diml, review
  by @rgrinberg).

- New binary kind `js` for executables in order to explicitly enable Javascript
  targets, and a switch `(explicit_js_mode)` to require this mode in order to
  declare JS targets corresponding to executables. (ocaml/dune#1941, @nojb)
@rgrinberg rgrinberg mentioned this pull request Jul 22, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aalekseyev aalekseyev aalekseyev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rgrinberg @aalekseyev