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 Windows Phone (C++/Cx) support #290

Open
wants to merge 53 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@DEGoodmanWilson
Contributor

DEGoodmanWilson commented Nov 20, 2016

The code in this PR is over a year old. I don't rightly know how hard this is going to be to merge. I imagine it's not going to be easy—there'll be changes that need to be made. Let's talk! I'm ready to finally get this in.

@smarx

This comment has been minimized.

smarx commented Nov 20, 2016

Automated message from Dropbox CLA bot

@DEGoodmanWilson, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here.

@smarx

This comment has been minimized.

smarx commented Nov 20, 2016

Automated message from Dropbox CLA bot

@DEGoodmanWilson, thanks for signing the CLA!

@gnichola

This comment has been minimized.

Contributor

gnichola commented Nov 29, 2016

@DEGoodmanWilson have you seen the blog MS released this morning? I know it is early, but would this change your thinking or do you think Cx is still a valid way to go?
https://blogs.windows.com/buildingapps/2016/11/28/standard-c-windows-runtime-cwinrt

@DEGoodmanWilson

This comment has been minimized.

Contributor

DEGoodmanWilson commented Nov 29, 2016

Well, that's pretty darned cool! I like Cx OK, but it's got its quirks (and was a huge step up on CLI), but this looks very very promising. That said, in the end, if you are developing with .NET assemblies, you might not care how those assemblies were built, and to the end user of djinni it might not make a whole lot of diffference, since all of the Cx code is auto-generated, So…

@DEGoodmanWilson

This comment has been minimized.

Contributor

DEGoodmanWilson commented Nov 29, 2016

(Also, I know I need to clean up the commit history a bit here! This was many months of effort, and there are some extraneous commits I need to remove from the PR)

@AndrewAtAvenza

This comment has been minimized.

AndrewAtAvenza commented Dec 13, 2016

I was actually about to start working on this but this looks pretty far along! I'm probably not the best person to fix the conflicts (I'm not even sure how since I definitely don't have write access) but I'd be happy to assist if possible. At the very least I could probably test this against our code base and see how that flies.

@steipete

This comment has been minimized.

Contributor

steipete commented Dec 22, 2016

We'd love to see this as well and I offer to help. We use djinni a lot and our C++ parts run on Windows as well - can try this branch with our setup as a larger test case once it's rebased - if that helps.

Don Goodman-Wilson added some commits Jun 28, 2015

Don Goodman-Wilson
Oh, so much closer. Now we are generating at least some of the code t…
…hat needs to be generated! The code is _wrong_ but it exists!

@DEGoodmanWilson DEGoodmanWilson force-pushed the DEGoodmanWilson:master branch from 70d95b7 to 2f2be44 Dec 22, 2016

@DEGoodmanWilson

This comment has been minimized.

Contributor

DEGoodmanWilson commented Dec 22, 2016

@steipete I've got this refactored so only the relevant contributions—and only my contributions—are included. I can't see what the conflicts are, curiously. Git is not cooperating today. Anyway, would absolutely love your feedback.

@artwyman

Not a full review yet by any stretch. I just skimmed the overall structure and changes to shared files to get a high-level impression of the changes. It's not even 100% clear to me whether C++/Cx is being added here as a front-end or back-end language, but I presume front-end (i.e. bridging C++/Cx to C++, not to Java or ObjC).

In general, I'm open to adding more languages like this, though if Microsoft is going to be supporting WinRT in standard C++ that's likely the approach I'd take for a new project. As far as merging this into master I'm going to be concerned about how "production ready" this new feature is, and how it might interact with other languages/use cases. If this code gets to the point of being ready for limited use, but not yet supporting everything, it could be put on a branch similar to Python, if you're willing to sign up to be the maintainer of that branch.

At a high level, I'd like to see more documentation of the new language, how it behaves, and any limitations or pre-requisites. I'd also like to see sample code and test cases, which are the best demonstration of how the generator actually works. Look at the Python branch for an example of those things.

The test-suite is a bit of a sticking point which has slowed merging of the python branch, and may make things difficult here as well. It's structured to assume that all languages are tested with the same input files, which breaks down if not all languages have exactly the same features. You can see on the Python branch how the test suite has been forked in an awkward way. Ideally it would be great if someone finds the time to restructure the test suite to run Djinni multiple times on different test inputs which can be configured per language.

"target_name": "djinni_cx",
"type": "static_library",
"sources": [
],

This comment has been minimized.

@artwyman

artwyman Dec 28, 2016

Member

Should list the header files, if nothing else. Not sure if that's relevant to whichever generator you're using for this, but it's the normal standard used in the other targets in this file.

@@ -194,6 +214,37 @@ object Main {
.text("Optional file in which to write the list of output files produced.")
opt[Boolean]("skip-generation").valueName("<true/false>").foreach(x => skipGeneration = x)
.text("Way of specifying if file generation should be skipped (default: false)")
opt[File]("cx-out").valueName("<out-folder>").foreach(x => cxOutFolder = Some(x))
.text("HAHAHAHAHAHAHAHAHAHA")

This comment has been minimized.

@artwyman

artwyman Dec 28, 2016

Member

Can you provide real documentation, please?

@@ -238,6 +298,10 @@ object Main {
cppIdentStyle = cppIdentStyle.copy(enumType = cppTypeEnumIdentStyle)
}
// if (cxTypeEnumIdentStyle != null) {

This comment has been minimized.

@artwyman

artwyman Dec 28, 2016

Member

Needed?

@@ -0,0 +1,86 @@
//

This comment has been minimized.

@artwyman

artwyman Dec 28, 2016

Member

Can the Cpp and Cx proxy caches be refactored to use the shared template impl in proxy_cache_*?

@@ -31,9 +31,9 @@ class JNIGenerator(spec: Spec) extends Generator(spec) {
val jniBaseLibClassIdentStyle = IdentStyle.prefix("H", IdentStyle.camelUpper)
val jniBaseLibFileIdentStyle = jniBaseLibClassIdentStyle
val writeJniCppFile = writeCppFileGeneric(spec.jniOutFolder.get, spec.jniNamespace, spec.jniFileIdentStyle, spec.jniIncludePrefix) _
val writeJniCppFile = writeCppFileGeneric(spec.jniOutFolder.get, spec.jniNamespace, spec.jniFileIdentStyle, spec.jniIncludePrefix, spec.cppExt, spec.cppHeaderExt) _

This comment has been minimized.

@artwyman

artwyman Dec 28, 2016

Member

I don't see where these new args are passed. Can you clarify what changes you're making to non-Cx generators, and how they're related?

@phraemer

This comment has been minimized.

phraemer commented Feb 8, 2017

For anyone else trying this out here are a few hints to get the example generating code you can use in a Visual Studio project to build a WinRT component called MyComponent that bridges your native C++ with the managed languages, C# etc.

I added this to run_djinni.sh

cxcpp_out="$base_dir/generated-src/cxcpp"
cx_out="$base_dir/generated-src/cx"
...
    --cpp-namespace MyComponent_CPP \
...
    --cxcpp-out "$temp_out/cxcpp" \
    --cx-out "$temp_out/cx" \
    --cx-namespace MyComponent \
    --cxcpp-namespace MyComponent_CXCPP \
    --cpp-include-prefix "generated-src/cpp/" \
    --cx-include-prefix "generated-src/cx/" \
    --cxcpp-include-prefix "generated-src/cxcpp/" \
    --cxcpp-include-cpp-prefix "generated-src/cpp/" \
...
mirror "cxcpp" "$temp_out/cxcpp" "$cxcpp_out"
mirror "cx" "$temp_out/cx" "$cx_out"

You need different namespaces for cx, cpp and cppcx or you will have problems in your WinRT component. The cx namespace should be the same as the name of your component.

In example.djinni

textbox_listener = interface +x {

In handwritten-src/cpp/sort_items_impl.hpp

#include "generated-src/cpp/sort_items.hpp"
#include "generated-src/cpp/textbox_listener.hpp"

namespace MyComponent_CPP {

In handwritten-src/cpp/sort_items_impl.cpp

namespace MyComponent_CPP {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment