djinni-support-lib icon indicating copy to clipboard operation
djinni-support-lib copied to clipboard

[DRAFT] Import wasm support from snapchat repo

Open mutagene opened this issue 2 years ago • 16 comments

mutagene avatar Nov 12 '23 18:11 mutagene

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 avatar Nov 12 '23 19:11 a4z

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 avatar Nov 12 '23 21:11 a4z

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 avatar Nov 12 '23 22:11 a4z

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 :-)

a4z avatar Nov 18 '23 13:11 a4z

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 avatar Nov 19 '23 12:11 mutagene

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 avatar Nov 19 '23 14:11 mutagene

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

  • [x] 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)
  • [x] 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..?

mutagene avatar Nov 23 '23 13:11 mutagene

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

a4z avatar Nov 23 '23 21:11 a4z

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: https://github.com/cross-language-cpp/djinni-generator/pull/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.

a4z avatar Nov 24 '23 19:11 a4z

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 avatar Nov 25 '23 14:11 mutagene

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.

mutagene avatar Nov 25 '23 15:11 mutagene

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.

a4z avatar Nov 26 '23 08:11 a4z

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.

mutagene avatar Nov 27 '23 20:11 mutagene

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 avatar Dec 10 '23 21:12 a4z

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 https://github.com/cross-language-cpp/djinni-support-lib/pull/77#issuecomment-1868088924

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 avatar Dec 22 '23 21:12 a4z

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: https://github.com/cross-language-cpp/djinni-generator/discussions/94#discussioncomment-7987560

a4z avatar Jan 01 '24 14:01 a4z