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 minimal example project (including CI) - take 2 #122

Merged
merged 4 commits into from
Oct 16, 2020

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Oct 13, 2020

This is an extension of #113.

I've re-based this PR in order to take advantage of #116 (Box<T> gets converted to *T).

I've also extended the Rust example code a bit, using an opaque pointer in the C API. I think this is a typical use case, isn't it?
I've also added a test program and a corresponding Makefile. I also added a README.md.

I think it is good to test the actual linking and usage of the generated library in CI, which is now done for Linux and macOS.

I have no idea how such a thing is typically done in Windows, but I guess this can be added later by somebody else.

I guess an actual C unit test framework could be used to test this, but for now I chose the simpler option of a small hand-written test program.

For now, only a simple Makefile is shown as an example for a build system, but at a later point other build systems (like e.g. CMake) could be added.

I've added a CI step that prints out the pkg-config results, any ideas how to more properly test this?

The auto-generated header file is installed to /usr/local/include/example_project/example_project.h. Is there a way to avoid creating a separate sub-directory and install it directly to /usr/local/include/example_project.h?

For now I've used this:

#include <example_project/example_project.h>

... but I think it would be more typical (at least for small projects) to do this:

#include <example_project.h>

@Luni-4
Copy link
Collaborator

Luni-4 commented Oct 14, 2020

@mgeier

Thanks for your changes! 😄

Could you please rebase?

To fix this CI problem instead:

Error: CliError { error: Some(Cannot copy D:\a\cargo-c\cargo-c\example-project\target\.\release\example_project.dll.lib to temp\usr\local\lib\example_project.dll.lib.

Caused by:
    The system cannot find the file specified. (os error 2)), exit_code: 101 }
Error: Process completed with exit code 1.

I guess we need to create the temp folder used by cinstall manually.

@mgeier
Copy link
Contributor Author

mgeier commented Oct 14, 2020

Could you please rebase?

Done.

I guess we need to create the temp folder used by cinstall manually.

Shouldn't cargo cinstall take care of this automatically?

@Luni-4
Copy link
Collaborator

Luni-4 commented Oct 14, 2020

I've also extended the Rust example code a bit, using an opaque pointer in the C API. I think this is a typical use case, isn't it?

Yep, it's a good solution

I have no idea how such a thing is typically done in Windows, but I guess this can be added later by somebody else.

Yes, we can extend it in another PR

For now, only a simple Makefile is shown as an example for a build system, but at a later point other build systems (like e.g. CMake) could be added.

i think a Makefile is enough, it's not necessary to add an additional build system for a single example file

I've added a CI step that prints out the pkg-config results, any ideas how to more properly test this?

For now it's fine, we could eventually add a test to compare the pkg-config output, but in another PR

The auto-generated header file is installed to /usr/local/include/example_project/example_project.h. Is there a way to avoid creating a separate sub-directory and install it directly to /usr/local/include/example_project.h?

For now I've used this:

#include <example_project/example_project.h>

... but I think it would be more typical (at least for small projects) to do this:

#include <example_project.h>

You can add pkg-config headers and libs paths to the Makefile.

Something like:

CFLAGS=`pkg-config --cflags example_project`
LDFLAGS=`pkg-config --libs example_project`
CC=gcc

all: example_project

example_project: example_project.o
     $(CC) -o $@ $(CFLAGS) $(LDFLAGS) $< 

In this way you can use #include <example_project.h> in your file.

@Luni-4
Copy link
Collaborator

Luni-4 commented Oct 14, 2020

Shouldn't cargo cinstall take care of this automatically?

Nope, the default value for destdir is this one while the directories created by cinstall are these ones

@mgeier
Copy link
Contributor Author

mgeier commented Oct 15, 2020

@Luni-4 Thanks for the review!

In the meantime, I've renamed the directory with the C test: e055202.
And I added a little more documentation: a86748b.

Shouldn't cargo cinstall take care of this automatically?

Nope, the default value for destdir is this one while the directories created by cinstall are these ones

I've decided this is too complicated for me (as a non-Windows-user), so I disabled the installation on Windows for now: 799aacb.
I hope that somebody else can fix this later.

I have no idea how such a thing is typically done in Windows, but I guess this can be added later by somebody else.

Yes, we can extend it in another PR

Perfect, this goes way beyond my abilities (and I don't really want to learn how to do it).

For now, only a simple Makefile is shown as an example for a build system, but at a later point other build systems (like e.g. CMake) could be added.

i think a Makefile is enough, it's not necessary to add an additional build system for a single example file

OK, cool.

I thought that maybe CMake could be used, because this might also work on Windows ... but this is beyond the scope of this PR.

I've added a CI step that prints out the pkg-config results, any ideas how to more properly test this?

For now it's fine, we could eventually add a test to compare the pkg-config output, but in another PR

OK, cool.

The auto-generated header file is installed to /usr/local/include/example_project/example_project.h. Is there a way to avoid creating a separate sub-directory and install it directly to /usr/local/include/example_project.h?
For now I've used this:

#include <example_project/example_project.h>

... but I think it would be more typical (at least for small projects) to do this:

#include <example_project.h>

You can add pkg-config headers and libs paths to the Makefile.

I know, but if the library is installed to the default location, I don't even need pkg-config.

And I don't see any advantage in having the header file in a sub-directory.

The only situation where this makes sense is if there are multiple header files.
Does cargo-c ever create multiple header files?

I think the default for single header files should be to not create a sub-directory.

Anyway, this can be further discussed and implemented at another time.
From my side, this PR is ready to be merged.
Is there still something missing?

@Luni-4
Copy link
Collaborator

Luni-4 commented Oct 15, 2020

@mgeier Thanks to you for your work!

Yes, we can implement the Windows stuff later.

I thought that maybe CMake could be used, because this might also work on Windows ... but this is beyond the scope of this PR.

Thinking about this more in deep, building the example with a build system could help people to figure out how to use it alongside cargo-c, so if you are interested in it, feel free to add that in another PR. I would advise the meson build system for its much simpler syntax.

I know, but if the library is installed to the default location, I don't even need pkg-config.

And I don't see any advantage in having the header file in a sub-directory.

The only situation where this makes sense is if there are multiple header files.
Does cargo-c ever create multiple header files?

I think the default for single header files should be to not create a sub-directory.

Anyway, this can be further discussed and implemented at another time.

Can we use the sub-directory approach for this example for now? As you said, we can always change that behavior later.

From my side, this PR is ready to be merged.
Is there still something missing?

After the new sub-directories Makefile approach is ready, please squash all commits into a single one and I'm going to review your work :)

@mgeier
Copy link
Contributor Author

mgeier commented Oct 15, 2020

building the example with a build system could help people to figure out how to use it alongside cargo-c, so if you are interested in it, feel free to add that in another PR. I would advise the meson build system for its much simpler syntax.

I think this is a good idea, but I'm not familiar with meson (nor with any other cross-platform build system), so somebody else should tackle this.

Can we use the sub-directory approach for this example for now?

Sure, currently that's the only way to do it, AFAICT.
For now, the header file will end up in /usr/local/include/example_project/example_project.h.

Getting rid of the unnecessary sub-directory is a separate task, I've just created an issue for that: #125.

After the new sub-directories Makefile approach is ready,

I don't understand what you mean by that.
Currently, the #include contains the sub-directory, which is IMHO the most reasonable thing to do until #125 is solved.

please squash all commits into a single one and I'm going to review your work :)

Done.

@Luni-4
Copy link
Collaborator

Luni-4 commented Oct 15, 2020

Can we use the sub-directory approach for this example for now?

Sure, currently that's the only way to do it, AFAICT.
For now, the header file will end up in /usr/local/include/example_project/example_project.h.

Getting rid of the unnecessary sub-directory is a separate task, I've just created an issue for that: #98

Perfect, thanks!

I don't understand what you mean by that.
Currently, the #include contains the sub-directory, which is IMHO the most reasonable thing to do until #125 is solved.

Using pkg-config in the Makefile, so you can use #include <example_project.h> instead of #include <example_project/example_project.h> in the C file

@mgeier
Copy link
Contributor Author

mgeier commented Oct 15, 2020

Using pkg-config in the Makefile, so you can use #include <example_project.h> instead of #include <example_project/example_project.h> in the C file

I don't think that's the best way to do it in this instructional example.

Since the library is installed into the default location, no pkg-config should be necessary.

If we would want to show the use of pkg-config, we should install the example library into a non-standard location. But why would we want to do that? Furthermore, we would also have to specify the path to pkg-config itself in the Makefile (which typically involves a PKG_CONFIG variable), which would unnecessarily complicate the situation.

I think we should show the most typical usage in this example, therefore I chose not to involve pkg-config.

The switch to #include <example_project.h> should only happen once #125 is resolved.

@Luni-4
Copy link
Collaborator

Luni-4 commented Oct 15, 2020

Using pkg-config in the Makefile, so you can use #include <example_project.h> instead of #include <example_project/example_project.h> in the C file

I don't think that's the best way to do it in this instructional example.

Since the library is installed into the default location, no pkg-config should be necessary.

If we would want to show the use of pkg-config, we should install the example library into a non-standard location. But why would we want to do that? Furthermore, we would also have to specify the path to pkg-config itself in the Makefile (which typically involves a PKG_CONFIG variable), which would unnecessarily complicate the situation.

I think we should show the most typical usage in this example, therefore I chose not to involve pkg-config.

The switch to #include <example_project.h> should only happen once #125 is resolved.

Seems legit, let's fix #125 first

Copy link
Collaborator

@Luni-4 Luni-4 left a comment

Choose a reason for hiding this comment

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

@mgeier

I addressed the points discussed above in this review

.github/workflows/example-project.yml Show resolved Hide resolved
.github/workflows/example-project.yml Show resolved Hide resolved
.github/workflows/example-project.yml Outdated Show resolved Hide resolved
.github/workflows/example-project.yml Show resolved Hide resolved
.github/workflows/example-project.yml Show resolved Hide resolved
.github/workflows/example-project.yml Show resolved Hide resolved
.github/workflows/example-project.yml Show resolved Hide resolved
.github/workflows/example-project.yml Outdated Show resolved Hide resolved
example-project/cbindgen.toml Show resolved Hide resolved
@Luni-4 Luni-4 merged commit b37d3e9 into lu-zero:master Oct 16, 2020
@mgeier mgeier deleted the example-project-take-2 branch October 16, 2020 15:13
@mgeier
Copy link
Contributor Author

mgeier commented Oct 16, 2020

Thanks for merging!
I've created an issue as a reminder for the Windows stuff: #128.

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.

2 participants