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

CMake install #14

Merged
merged 3 commits into from
Sep 3, 2020
Merged

CMake install #14

merged 3 commits into from
Sep 3, 2020

Conversation

a4z
Copy link

@a4z a4z commented Aug 5, 2020

targets issue #13

Miss tests that this work, would need:

generate some code from a djinni interface file, do something with it, and build a test program.
and this on Windows Linux OSX iOS and Android

not sure how to implement that, maybe not all at once.
also, for such a test is it somehow sub optimal to always also build the generator, this should for this test actually be consumed as a ready to use package/binary

So this is for a base of discussion
and you can allreay, if wanted, checkout and see if this works

@a4z a4z requested review from jothepro and bojanin August 5, 2020 17:10
@jothepro
Copy link

jothepro commented Aug 5, 2020

I like that! But I'm not sure if i got it how this would be used. I guess the idea is that I would have to call make install once when building djinni on my machine, and then the header-files will be available globally for all my projects?

generate some code from a djinni interface file, do something with it, and build a test program.

Afaik that is basically what the current tests in the test-suite do (At least for Linux/Java and macOS/objc). I think it should be enough to write a CMake-script that generates, builds and runs the existing test-cases utilizing the installed header files. That would proof that the install is working.

also, for such a test is it somehow sub optimal to always also build the generator, this should for this test actually be consumed as a ready to use package/binary

Yeah, it is super annoying how djinni is built/run by default. I think a proper build should always result in a standalone jar artifact, which is not the default when building djinni right now. It should be pretty easy to change the build behaviour though, the build-target for generating a jar already exists.

@a4z
Copy link
Author

a4z commented Aug 5, 2020

I like that! But I'm not sure if i got it how this would be used. I guess the idea is that I would have to call make install once when building djinni on my machine, and then the header-files will be available globally for all my projects?

the headers, and the library to link against.
A project using jinni does not necessarily require the code generator, I could generate the code once, add it to the project, and I am done with the generator. (I do not say that this is a good way;-)
But the support lib is always required for building.

And this is for me the main difference.
There will never be a iOS or Android build of the generator, but there are iOS and Android builds of the support lib.
So these are different dependencies, the generator is a buildhost dependency, the support lib a target dependency of a project.

And these are different tests, generator is text in, text out test, basically it should be enough to to this on one platform since the same java artifact can be used on Linux OSX Windows, hopefully.

But the library needs to be compiled, and executed on each of the possible target platforms, and these are quite many.
Windows Linux OSX, iOS arm64, arm64e, x86_64, mabye arm7 (tv/watch), and 4 Android architectures,

so for me the generator and the support lib are 2 total different entities, and I am not sure on how to handle and respect this in the build && testing pipeline.

@jothepro
Copy link

jothepro commented Aug 5, 2020

There will never be a iOS or Android build of the generator, but there are iOS and Android builds of the support lib.

In the last hour I've been thinking about what you try to achieve with #13, now I get it! Sounds interesting, it ultimately means that it could make sense to distribute generator and support lib separately 😮 (jar via github release and/or mavencentral, lib via package manager conan, hunter, ...). Neat.

generator is text in, text out test, basically it should be enough

I doubt that these tests exist, all that I have seen is integration-tests that also build and run the resulting code (Just a comment, I don't want to imply anything with that).

Windows Linux OSX, iOS arm64, arm64e, x86_64, mabye arm7 (tv/watch), and 4 Android

What makes you want to test the support-lib that extensively? My naive thinking is that the support-lib does not include any platform-specific code, so it should be enough to test it once for java and objc. I don't expect different behaviour depending on the CPU architecture / operating system.

@a4z
Copy link
Author

a4z commented Aug 6, 2020

In the last hour I've been thinking about what you try to achieve with #13, now I get it! Sounds interesting, it ultimately means that it could make sense to distribute generator and support lib separately 😮 (jar via github release and/or mavencentral, lib via package manager conan, hunter, ...). Neat.

exactly, but this also raises the question, how to avoid to run useless tests, since when I change something at one, why should the other be tested?

I doubt that these tests exist, all that I have seen is integration-tests that also build and run the resulting code (Just a comment, I don't want to imply anything with that).

Then they should exists. but maybe they do? why else should the generated-src be checked in into git
https://github.com/cross-language-cpp/djinni/tree/master/test-suite/generated-src

What makes you want to test the support-lib that extensively? My naive thinking is that the support-lib does not include any platform-specific code, so it should be enough to test it once for java and objc. I don't expect different behaviour depending on the CPU architecture / operating system.

beside normal paranoia , you mean? :-)
the support lib has maybe not (yet) platform specific logic, but the cmake file has.
When the cmake file will change, what is very likely going to happen, you want to ensure that no header is missing out, for example. And there might be more (in future), talking about cmake modules to find the library.

We do not have to have everything in all combination and perfect at the beginning now, but I would like to point this into the right direction and use this to raise some discussion topics, like 2 packages / components we have.

@a4z a4z marked this pull request as ready for review August 18, 2020 12:34
@a4z
Copy link
Author

a4z commented Aug 25, 2020

Hi @jothepro and / or @bojanin , are you still around ?

@jothepro
Copy link

Sorry, I didn't realize you where awaiting further review/feedback!

Copy link

@jothepro jothepro left a comment

Choose a reason for hiding this comment

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

Do you still think about extracting the support-lib into it's separate library? The more I am thinking about it the more I like the idea. Only gotcha I can think of would be that the releases would be de-coupled.

.gitignore Outdated Show resolved Hide resolved
@jothepro
Copy link

Do you think it makes sense to adjust the existing tests, so they use the target instead of globbing for the files: https://github.com/cross-language-cpp/djinni/blob/master/test-suite/java/CMakeLists.txt#L27
This could serve as some basic test/documentation on how to use the installed target.

Co-authored-by: jothepro <github@jothe.pro>
@a4z
Copy link
Author

a4z commented Aug 25, 2020

Do you still think about extracting the support-lib into it's separate library? The more I am thinking about it the more I like the idea. Only gotcha I can think of would be that the releases would be de-coupled.

still not 100% for one or the other, there are reasons for both ,I guess,
but form a CI/CD POV, why running all tests for A if changes happen in B, and vice versa ...

and these are 2 projects, on the other hand ... they are pretty strong related

Do you think it makes sense to adjust the existing tests, so they use the target instead of globbing for the files: https://github.com/cross-language-cpp/djinni/blob/master/test-suite/java/CMakeLists.txt#L27
This could serve as some basic test/documentation on how to use the installed target.

I am not sure I fully get what you mean.
The globbing for the support lib can for sure be replaced by file names

Those for the test suite also, even if I understand why this has been done,

but this would be an extra ticket
(looking at the CMake , there are some other tasks also, it seems, but this is outside of this PR)

Copy link

@jothepro jothepro left a comment

Choose a reason for hiding this comment

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

I'm open for discussion about this, but I believe a simple install-target is not enough.

CMakeLists.txt Show resolved Hide resolved
@jothepro jothepro self-requested a review August 26, 2020 16:43
@a4z a4z merged commit cd60ea5 into cross-language-cpp:master Sep 3, 2020
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.

3 participants