Skip to content

Ctypes: adds a stop-gap measure deps fields#5346

Merged
bobot merged 7 commits intoocaml:mainfrom
bobot:ctypes_dependencies
Mar 10, 2022
Merged

Ctypes: adds a stop-gap measure deps fields#5346
bobot merged 7 commits intoocaml:mainfrom
bobot:ctypes_dependencies

Conversation

@bobot
Copy link
Copy Markdown
Collaborator

@bobot bobot commented Jan 16, 2022

The deps fields added to the ctypes sub-stanza, is not meant to be in a final version of the ctypes feature. But I think it could help to experiment with it by fixing missing dependencies. Cf #5325

There is also a dirty hack for #5190 so that I can test it.

CC @mbacarella

@bobot bobot force-pushed the ctypes_dependencies branch from 025978d to 4b9df71 Compare January 16, 2022 20:38
@bobot bobot requested a review from christinerose as a code owner January 16, 2022 20:38
@bobot
Copy link
Copy Markdown
Collaborator Author

bobot commented Jan 16, 2022 •
edited
Loading

There is also a fix for allowing the use of :standard in c_flags of the ctypes sub-stanza. @voodoos, there is nothing to do for link flags?

@voodoos
Copy link
Copy Markdown
Collaborator

voodoos commented Jan 17, 2022

@voodoos, there is nothing to do for link flags?

I dit not look in depth at the new ctypes field and am not aware of it's functioning so the question is a bit vague to me.

But one thing we should probably be careful about is, when building foreign stubs in C++, to have the flags Cxx_flags.get_flags ~for_:Link <dir> in the :standard set of link flags. (we don't have such specific flags for C compilation).

Initial issue: #4846
PR fixing fix for classic foreign stubs: #5185

@bobot bobot force-pushed the ctypes_dependencies branch from 4b9df71 to 7f9731b Compare January 17, 2022 16:50
@bobot
Copy link
Copy Markdown
Collaborator Author

bobot commented Jan 18, 2022

Thank you for the confirmation that there is nothing to add for C. I think that Ctypes only create C files (that don't use C++ datastructures) so even if one create a stub for a C++ library I think we don't need to do anything differently than for other libraries . The C++ library is a library dependency like gmp could be one.

christinerose
christinerose approved these changes Jan 18, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@christinerose christinerose left a comment

Choose a reason for hiding this comment

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

Just a few minor things in syntax, a spelling error, and suggested formatting for ctypes library name.

@bobot
Copy link
Copy Markdown
Collaborator Author

bobot commented Jan 19, 2022 •
edited
Loading

NTS: add -Werror=implicit-function-declaration for the compilation of the stub in order to catch function name misspelling.

@bobot bobot changed the title [Draft] Ctypes: adds a stop-gap measure deps fields Ctypes: adds a stop-gap measure deps fields Feb 9, 2022
christinerose
christinerose suggested changes Feb 11, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@christinerose christinerose left a comment

Choose a reason for hiding this comment

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

Here are a few minor suggestions for subject/verb agreement and fixing a typo. Hope it helps!

@bobot bobot force-pushed the ctypes_dependencies branch from 4c9fdc4 to eec0f4e Compare February 19, 2022 22:17
@bobot bobot requested a review from rgrinberg February 19, 2022 22:22
@bobot bobot force-pushed the ctypes_dependencies branch from eec0f4e to 72e53b3 Compare February 19, 2022 22:24
@bobot bobot mentioned this pull request Feb 21, 2022
Closed
christinerose
christinerose suggested changes Feb 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@christinerose christinerose left a comment

Choose a reason for hiding this comment

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

Just a few minor syntax / verb agreement suggestions

@bobot
Copy link
Copy Markdown
Collaborator Author

bobot commented Feb 23, 2022

@mbacarella can you review this MR?

@mbacarella
Copy link
Copy Markdown
Collaborator

mbacarella commented Feb 23, 2022

@mbacarella can you review this MR?

lgtm aside from one question: why do you need to instantiate and pass around a sandbox to support this deps feature? Shouldn't whatever you depend on get surfaced from the sandbox if it's listed as a target?

bobot and others added 7 commits March 9, 2022 23:38
@bobot
[Ctypes] Add deps field for fixing dependencies on local files
67445d8 
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Ctypes] expand the c_flags and c_library_flags
9a141c1 
         before writing them to sexp

Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Doc] Some precisions about -fpic and .so
7044bb8 
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot @christinerose
Update doc/foreign-code.rst
205c92e 
Co-authored-by: Christine Rose <christinerose@users.noreply.github.com>
@bobot @christinerose
Update doc/foreign-code.rst
26f0b43 
Co-authored-by: Christine Rose <christinerose@users.noreply.github.com>
@bobot @christinerose
Fix doc wording
ea3a1d9 
Co-authored-by: Christine Rose <christinerose@users.noreply.github.com>
@bobot @christinerose
Fix doc wording
5c25ecf 
Co-authored-by: Christine Rose <christinerose@users.noreply.github.com>
@bobot bobot force-pushed the ctypes_dependencies branch from 9d00eaf to 5c25ecf Compare March 9, 2022 22:44
@bobot
Copy link
Copy Markdown
Collaborator Author

bobot commented Mar 9, 2022

The passing around of the sandbox is just because the deps field generally allows to specify the kind of sandbox one desire.

@bobot bobot merged commit 6cd4d1d into ocaml:main Mar 10, 2022
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Apr 15, 2022
@rgrinberg
[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, du…
ba3c1b9 
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info and dune-action-plugin (3.1.0)

CHANGES:

- Add `sourcehut` as an option for defining project sources in dune-project
  files. For example, `(source (sourcehut user/repo))`. (ocaml/dune#5564, @rgrinberg)

- Add `dune coq top` command for running a Coq toplevel (ocaml/dune#5457, @rlepigre)

- Fix dune exec dumping database in wrong directory (ocaml/dune#5544, @bobot)

- Always output absolute paths for locations in RPC reported diagnostics
  (ocaml/dune#5539, @rgrinberg)

- Add `(deps <deps>)` in ctype field (ocaml/dune#5346, @bobot)

- Add `(include <file>)` constructor to dependency specifications. This can be
  used to introduce dynamic dependencies (ocaml/dune#5442, @anmonteiro)

- Ensure that `dune describe` computes a transitively closed set of
  libraries (ocaml/dune#5395, @esope)

- Add direct dependencies to $ dune describe output (ocaml/dune#5412, @esope)

- Show auto-detected concurrency on Windows too (ocaml/dune#5502, @MisterDA)

- Fix operations that remove folders with absolute path. This happens when
  using esy (ocaml/dune#5507, @EduardoRFS)

- Dune will not fail if some directories are non-empty when uninstalling.
  (ocaml/dune#5543, fixes ocaml/dune#5542, @nojb)

- `coqdep` now depends only on the filesystem layout of the .v files,
  and not on their contents (ocaml/dune#5547, helps with ocaml/dune#5100, @ejgallego)

- The mdx stanza 0.2 can now be used with `(implicit_transitive_deps false)`
  (ocaml/dune#5558, fixes ocaml/dune#5499, @emillon)

- Fix missing parenthesis in printing of corresponding terminal command for
  `(with-outputs-to )` (ocaml/dune#5551, fixes ocaml/dune#5546, @Alizter)
@rgrinberg rgrinberg mentioned this pull request Apr 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

1 more reviewer

@christinerose christinerose christinerose approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bobot @voodoos @mbacarella @christinerose