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.

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

Document that the capi feature must be present in the Cargo.toml #152

Open
mgeier opened this issue Dec 14, 2020 · 7 comments
Open

Document that the capi feature must be present in the Cargo.toml #152

mgeier opened this issue Dec 14, 2020 · 7 comments
Labels
F-Documentation Related to the documentation

Comments

@mgeier
Copy link
Contributor

mgeier commented Dec 14, 2020

If my Cargo.toml does not contain a [features] section defining the capi feature, the header file is generated correctly, but the dynamic library does not contain the expected symbols.

The README says:

You may use the feature capi to add C-API-specific optional dependencies.

... but it doesn't say that defining this feature is required.

UPDATE: I'm trying this on Linux, btw.

@lu-zero
Copy link
Owner

lu-zero commented Dec 14, 2020

Do you have your code available so I can take a look?

@mgeier
Copy link
Contributor Author

mgeier commented Dec 14, 2020

I've created #153 as an example.

The error appears in CI.

@lu-zero
Copy link
Owner

lu-zero commented Dec 15, 2020

It works as intended.

cargo rejects setting features that aren't present in the manifest. I cannot work around this limitation without changing the cargo API.

@lu-zero lu-zero added the F-Documentation Related to the documentation label Dec 15, 2020
@lu-zero lu-zero changed the title Generated library is missing symbols when "capi" feature is not defined Document that the capi feature must be present in the Cargo.toml Dec 15, 2020
@mgeier
Copy link
Contributor Author

mgeier commented Dec 16, 2020

This can be quite confusing, because the header contains all the functions, but the library doesn't.
Would it at least be possible to have the header file and the library contain (or not contain) the same functions?

The instruction "... use #[cfg(feature="capi")] to hide it when you build a normal rust library" from the README should not be separated from the instruction "You may use the feature capi to add C-API-specific optional dependencies.", because those two are not independent.

Still, even with updated documentation, this might cause a lot of confusion in the future, it would be great if it were possible to avoid the problem completely ...

What was the reason for deprecating #[cfg(cargo_c)]?
Using this would avoid the problem.

@lu-zero
Copy link
Owner

lu-zero commented Dec 16, 2020

there are two issues:

  • setting cfg(cargo_c) works but not for the tests because of how cargo rustc works, relaxing this constraint requires patching cargo.
  • adding on the fly features to the manifest requires patching cargo to add a setter for it.

On top cbindgen is happy to ignore cfg(feature="capi").

Possibly I can send patches to upstream cargo to get those two sorted.

cschwan added a commit to NNPDF/pineappl that referenced this issue Aug 10, 2021
@mgeier
Copy link
Contributor Author

mgeier commented Jan 10, 2022

In the meantime, the README contains the information that the capi feature is obligatory.

But maybe it would be helpful to explicitly mention how to do that?

Something like this:

[features]
capi = []

@lu-zero
Copy link
Owner

lu-zero commented Jan 11, 2022

It sounds a good idea indeed. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-Documentation Related to the documentation
Projects
None yet
Development

No branches or pull requests

2 participants