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

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

Merged
merged 7 commits into from
Mar 10, 2022
Merged

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

merged 7 commits into from
Mar 10, 2022

Conversation

bobot
Copy link
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
Copy link
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
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
Copy link
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
Collaborator

@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.

doc/foreign-code.rst Outdated Show resolved Hide resolved
doc/foreign-code.rst Outdated Show resolved Hide resolved
@bobot
Copy link
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 requested changes Feb 11, 2022
View reviewed changes
Copy link
Collaborator

@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!

doc/foreign-code.rst Outdated Show resolved Hide resolved
@bobot bobot requested a review from rgrinberg February 19, 2022 22:22
@bobot bobot mentioned this pull request Feb 21, 2022
Closed
christinerose
christinerose requested changes Feb 22, 2022
View reviewed changes
Copy link
Collaborator

@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

doc/foreign-code.rst Outdated Show resolved Hide resolved
doc/foreign-code.rst Outdated Show resolved Hide resolved
@bobot
Copy link
Collaborator Author

bobot commented Feb 23, 2022

@mbacarella can you review this MR?

@mbacarella
Copy link
Collaborator

@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
Copy link
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

@christinerose christinerose christinerose approved these changes

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

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