-
Notifications
You must be signed in to change notification settings - Fork 10
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
[DRAFT] Import wasm support from snapchat repo #76
base: main
Are you sure you want to change the base?
Conversation
We need to provide a generator with the changes, I guess My current plan is to switch to a CMake fetch_content way of working. Please be a little bit patient. This might look like a lot of what is happening, but this will set the base for a maintainable workflow for the following years. And I wish that the WASM functionality would be part of it. |
btw the CMake part to get the generator looks atm like this to me
This gives me the latest developer release (current main branch) For getting the build artifact from a pipeline, this needs to change a bit, I will check and report back. Something like this will be the (from me) recommended way of using the generator. |
Downloading a pipeline artifact is not as straightforward since they are not publicly available. it only works as an authenticated user, which is not that awesome, but solvable I don't know right now what the most user-friendly way would be. |
4c8b976
to
5a4a208
Compare
I just checked. Downloading a build artifact works
The artifact_id can be read from the download link, Then unzip djinni.zip, give it x permission, and everything is fine. I make settings on the repos to make it possible to generate fine-graded personal tokens that only are allowed to download a build artifact. That's good since that means I can add my token to the CI However, I have to think about how to most elegant enable that in a ci run of the support lib. The dynamic properties of the artifact ID generation make that a little bit inelegant, but at least there is a way It's nice to finally have a reason to dig into that situation :-) |
I just noticed that the test-suite unit tests for wasm/ts are not being run on the snapchat CI. I hope we can get it running here. |
The unit tests are running locally. So all that remains (??) for this PR to complete the basic wasm support, I think, would be to run the wasm tests on the CI - which as I wrote above, snapchat-djinni is not doing currently - and add some kind of simple example which uses it. |
I've added the text sort example from the snapchat repo (as it was adapted from dropbox/djinni). It's building and running ok for me locally. So, still remaining
|
Thanks a lot, Alexis, this is now in an exciting state! I am really happy and thankful for the process. About the open points: run wasm tests on CI (and examples) Not sure how to verify the examples. They are defacto also tests, but would need GUI. documentation generator build: DjinnniModule.ts/js files as far as I see no real blocker, but some decisions need to be done, and some time for implementation need to be found, want might be the bigger problem, but I have that now on the top of my prio list |
69b168c
to
218ba9c
Compare
I started with a Python script to get the generator from a PR; it works locally with an access key in the environment. See #77
We need to setup the CI/CD env, so it has a key and then check that this temporary solution will not go into main, I hope to be able to do that, but my calendar is very stressful at the moment, so no promise, but I will do my best. |
5976b8c
to
6af3123
Compare
14821af
to
9f679e0
Compare
They can be created by the generator. This was my preferred approach, but in the generator integration tests we generate multiple sets of sources from djinni IDL, and for each DjinniModule.ts/js was being created - leading to problem with the cmake. Not sure how best to work this, but am tempted to make it so that whether or not to generate the DjinniModule.ts/js files can be specified as a command line parameter, and thus we can make sure only one set gets generated. |
This looks pretty nice now! Besides providing a unified way to get the wanted generator in a PR here, I also want to check that all the samples work and see if they can be used. Also, should they stay here, or should we move them into the https://github.com/cross-language-cpp/djinni-example-cc project (which should be revitalized as the demo project that utilizes all various generators) Super exciting, the outcome of that PR could be not only a new language generator but also severe infrastructure improvements and modernizations. I have a super busy week ahead, so please be patient if I am not immediately available for further actions in the upcoming days. |
13563f3
to
e945626
Compare
I'll probably slow down a bit. I'm keen to get this merged, and I think most of the hard work is done, but I also have a lot on my plate. |
I am sorry, but looking at my calendar, it is very likely my review must wait until the Christmas holidays. |
Had today time, tried various things, Need to think a bit, maybe we can live with pre-release and merge the PR on the generator, of we have some other idea. |
FYI: |
No description provided.