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

Add the metadata needed by cargo-c #176

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lu-zero
Copy link

@lu-zero lu-zero commented Aug 31, 2024

Fixes #174

It does not install any header, uses libz_rs as name in pkg-config and z_rs as library name.

The default cargo cinstall --destdir=/some/place would produce this on macos:

└── usr
    └── local
        └── lib
            ├── libz_rs.0.2.1.dylib
            ├── libz_rs.0.2.dylib -> libz_rs.0.2.1.dylib
            ├── libz_rs.a
            ├── libz_rs.dylib -> libz_rs.0.2.1.dylib
            └── pkgconfig
                └── libz_rs.pc

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.98%. Comparing base (85bc778) to head (9b4809f).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #176      +/-   ##
==========================================
- Coverage   92.47%   91.98%   -0.49%     
==========================================
  Files          41       44       +3     
  Lines       14174    14835     +661     
==========================================
+ Hits        13107    13646     +539     
- Misses       1067     1189     +122     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +104 to +113
> tree target
target
├── CACHEDIR.TAG
└── x86_64-unknown-linux-gnu
└── release
├── libz_rs.a
├── libz_rs.d
├── libz_rs.pc
├── libz_rs.so
└── libz_rs-uninstalled.pc
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lu-zero is there a reason cargo cbuild puts its things in target/<target tripple>/release rather than just target/release? it's annoyingly inconsistent with a standard cargo build.

Copy link
Author

Choose a reason for hiding this comment

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

It is required until cargo learns to differentiate the RUSTFLAGS for the build.rs and the RUSTFLAGS for the host.

using cargo cbuild -v it emits a note with the uninstalled pkg-config path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

are you aware of a cargo issue that describes the problem?

I'll be meeting with some members of the cargo team in October, and I'd like to prepare some concrete things to make progress on. Given how crucial this is for sneaking rust into C code bases I think progress can be made.

Copy link
Author

@lu-zero lu-zero Sep 4, 2024

Choose a reason for hiding this comment

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

rust-lang/cargo#4423 is another way to see the problem and rust-lang/cargo#9452 is how to solve it.

But normally cinstall is used to produce the .a that is then linked in the bigger C project. Even C projects support cross compilation ^^.

I guess I should really prepare a blogpost showing how to use cargo-c in many deployment scenarios :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please use cargo-c to produce the cdylib
2 participants