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

[DRAFT] Import wasm support from snapchat repo #76

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

Conversation

mutagene
Copy link

No description provided.

@a4z
Copy link
Contributor

a4z commented Nov 12, 2023

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.
This will fetch a build artifact from a PR, which can be used until the change in the generator has been released.

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.
I will work as quickly as possible and help as much as my day job and family situation allow me to do.

@a4z
Copy link
Contributor

a4z commented Nov 12, 2023

btw the CMake part to get the generator looks atm like this to me

set(Djinni_URL "https://github.com/cross-language-cpp/djinni-generator/releases/download/current-latest/djinni")
set(Djinni_DownloadPath "${CMAKE_BINARY_DIR}/djinni")

# get the generator and make it executable
if(NOT EXISTS ${Djinni_DownloadPath})
    message(STATUS "Downloading djinni from ${Djinni_URL}")
    file(DOWNLOAD ${Djinni_URL} ${Djinni_DownloadPath})
    file(CHMOD ${Djinni_DownloadPath} PERMISSIONS OWNER_EXECUTE OWNER_WRITE OWNER_READ)
endif()

# run djinni to see the command exists and works
execute_process(COMMAND ${CMAKE_BINARY_DIR}/djinni --version RESULT_VARIABLE result OUTPUT_VARIABLE output ERROR_VARIABLE error)
# print the result so we see what's going on and what works or not
message(STATUS "\$?: ${result}")
message(STATUS "djinni --version: ${output}")
message(STATUS "Error: ${error}")

This gives me the latest developer release (current main branch)

For getting the build artifact from a pipeline, this needs to change a bit,
since the pipeline will upload a zip of the generator, and fetch_content might be the better choice for handling that

I will check and report back.

Something like this will be the (from me) recommended way of using the generator.
Of course, with a checksum check.

@a4z
Copy link
Contributor

a4z commented Nov 12, 2023

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
https://docs.github.com/en/rest/actions/artifacts?apiVersion=2022-11-28#download-an-artifact

I don't know right now what the most user-friendly way would be.
Getting the generator from a PR for a PR in this repo
I will stay on it since this is a trivial problem that is only complicated due to the GH infrastructure.
And it's been bothering me since the repo split.
(Sorry for abusing this PR to brainstorm for a solution, But I see this now as a good motivator and use case to invest more energy in that situation finally)

@a4z
Copy link
Contributor

a4z commented Nov 18, 2023

I just checked. Downloading a build artifact works

ARTIFACT_ID=1049780945

curl -L \
  -H "Accept: application/vnd.github+json" \
  -H "Authorization: Bearer $TOKEN" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  https://api.github.com/repos/cross-language-cpp/djinni-generator/actions/artifacts/$ARTIFACT_ID/zip --output djinni.zip

The artifact_id can be read from the download link,
The $TOKEN, I can add one to the CI settings, but it will expire from time to time,

Then unzip djinni.zip, give it x permission, and everything is fine.
(on Windows rename to bat)

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.
But it is not a problem technically, it's just a question of what the 'best way' would be.

The dynamic properties of the artifact ID generation make that a little bit inelegant, but at least there is a way
Maybe there is even a chance to get the latest artifact to a PR, that would be nice, will continue look at the github API docs

It's nice to finally have a reason to dig into that situation :-)

@mutagene
Copy link
Author

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.

@mutagene
Copy link
Author

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.

@mutagene
Copy link
Author

mutagene commented Nov 23, 2023

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

  • build & run wasm tests on CI
  • documentation
  • get the djinni generator as required (for now, I might just add the binary directly to this branch for my testing)
  • I waffled on where to but the DjinnniModule.ts/js files. Conceptually they belong in the support lib, but I'm not sure in practice how a consuming application would be best to get these. Outside of git submodules of ad-hoc solutions, I guess the TS could also be included in a conan recipe - but if we're not updating to conan 2..?

@a4z
Copy link
Contributor

a4z commented Nov 23, 2023

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)
It seems that would require a headless chrome instance?

Not sure how to verify the examples. They are defacto also tests, but would need GUI.
I know it's possible to run both, Android and iPhone simulator/emulator headless, but would need to look that up.
It would be awesome to have that, but this requires more investigation. And it will drastically extend CI times. But soooo nice to have.

documentation
That might be the easiest part, I will happily contribute it, once I had time stepping through the additions.

generator build:
I am on the task of providing different ways for getting the generator into the support lib builds
The conan way is dead, and I will add a CMake download.
That can be the latest developer release, the latest tagged release, or, and this is what is missing, an artifact id
(artifact from an action as attached here: https://github.com/cross-language-cpp/djinni-generator/actions/runs/6964234110)
It's unfortunate that getting the artifact is on github more complex than required, but there is a way.

DjinnniModule.ts/js files
Could they be 'generated' by the generator? As far as I seen, those are just one liners, one for js one for ts, or do I miss something?
The Python bindings would require the same, but in the Python case the generated code would need adoption depending on the IDL input. So it's not static. I guess the js/ts is static and never changes?


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

@mutagene mutagene force-pushed the wasm-support branch 4 times, most recently from 69b168c to 218ba9c Compare November 24, 2023 00:47
@a4z a4z mentioned this pull request Nov 24, 2023
@a4z
Copy link
Contributor

a4z commented Nov 24, 2023

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
Unfortunately, GitHub requires such a workaround, but at least we can do it.

python ../get-generator-from-pr.py 157 will download the last successful build from the pr here: cross-language-cpp/djinni-generator#157.

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,
but when everything is ready, merge generator PR and then switch to the latest developer release of the generator.

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.

@mutagene mutagene force-pushed the wasm-support branch 4 times, most recently from 5976b8c to 6af3123 Compare November 25, 2023 14:19
@mutagene mutagene force-pushed the wasm-support branch 2 times, most recently from 14821af to 9f679e0 Compare November 25, 2023 14:40
@mutagene
Copy link
Author

mutagene commented Nov 25, 2023

Screenshot 2023-11-25 153923

WASM/TS tests are now running in a headless chrome instance, and failing test output is captured from the browser and dumped to the terminal.

@mutagene
Copy link
Author

DjinnniModule.ts/js files
Could they be 'generated' by the generator? As far as I seen, those are just one liners, one for js one for ts, or do I miss something?

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.

@a4z
Copy link
Contributor

a4z commented Nov 26, 2023

This looks pretty nice now!
I will have to give it a closer look, though.

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.

@mutagene mutagene force-pushed the wasm-support branch 8 times, most recently from 13563f3 to e945626 Compare November 26, 2023 23:31
@mutagene
Copy link
Author

I have a super busy week ahead, so please be patient if I am not immediately available for further actions in the upcoming days.

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.

@a4z
Copy link
Contributor

a4z commented Dec 10, 2023

I am sorry, but looking at my calendar, it is very likely my review must wait until the Christmas holidays.
Also, adding the generator selection in the pipeline is easy to do but requires some time.
I want to prioritize these tasks, and I will try to do it.
Thank you for your understanding and patience.

@a4z
Copy link
Contributor

a4z commented Dec 22, 2023

Had today time, tried various things,
deciding between the generator from the latest release, or the pre-release is possible.
Getting a generator from a PR is technically possible, practically not because atm I see now way to pass the access token to the PR. And strangely, github wants an access token when I want to download a build artifact via the API
See
#77 (comment)

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.
Other people do some crazy workarounds to skip this limitation, not sure if I want to adopt one of those

@a4z
Copy link
Contributor

a4z commented Jan 1, 2024

FYI:
I hope there should soon be a way to get the support lib, and generator workflow synced, and it will be possible to show a supporting lib works with a new generator without having a generator released.

Details: cross-language-cpp/djinni-generator#94 (comment)

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