C tooling

[bitwitch]

• cli tool rewritten in C
• building the cli tool and runtime in CI
• creating releases in CI when a vX.X.X tag is pushed
• the cli tool now has an update command to install the latest orca version

[martinfouilleul]

Hey, it's great! Here are some notes:

Tool

• bundle
  • Do we really want to let users override system orca dir? It seems like it could lead to using some version of the tool with another version of the source/libs.
  • There doesn't seem to be a --resource-file option like in the original python tool, only --resource-dir. Sometimes it's useful to just put single files (or dirs) inside the app's data dir.
  • mtlEnableCapture does not invoke frame capture itself as the help string suggests. It just enables it so you can take a frame capture in XCode.
  • For making the icon on macos, you can look at how it's done in the original script, and shell out to sips and iconutil.
• version the help string is the same as sdk-path
• update
  • This doesn't update the tool itself. Since our way of packaging stuff might change, the tool should probably first check if a new version of itself exists, and if so replace itself with it and re-run update with the new version.
  • We'd probably need more informative progress messages, but that can come later.

CI

• We talked about building angle directly from the orca repo on each release (instead of using a separate repo), which would make sure the angle version is always in sync with the orca release.
• Why is there source in release in addition to sources inside the tarball?
• The dev tool puts ANGLE libs in src/ext/..., which is how we used to do, but I'm thinking maybe we should put them in bin, and only have source files in ext? At some point the dev tool should install stuff in a dev_xyz version folder next to the user tool, so it would probably make more sense to not put binaries in src anyway

Misc

• Do we still use/need scripts/source.py ?
• In the tooling and the platform layers, you include platform specific stuff to win32.c or system.c ? I'd rather have fewer header files doing less include indirection internally. If there's a need to include "all platform stuff but no graphics" or things like that we can add ifdef selectors in orca.h/orca.c... What do you think?
• Some files have formatting that's a little inconsistent with our clang-format config, eg braces on the right, etc. and some have windows line endings (I never remember the git incantations for that, it's a pain...)
• win32_path.c in canonicalize, I don't know how you're supposed to handle long path names on Windows, I think MAX_PATH can be overflowed in some cases? what to do in this case?
• What's the reason for the changes in ui.c? Did MSVC complain, or is it to avoid a buggy conversion (in the latter case should the macro be made more explicit wrt conversions)? Anyway we should probably split that out from this PR.
• Remove or rename oc_str8_printf. There is oc_str8_ip (for "int, pointer") and oc_str8_lp (for "long long, pointer") that does the same thing already. I reckon these are not the best possible names, but oc_str8_printf isn't great either.

TODOs

(just making a note for myself of what's left before we can use this both in dev and user modes)

• Dev tooling: build stuff and install them in a dev directory in the user tooling tree
• Update the examples to use the new tooling
• Update the readme and doc (and split "using" and "building" docs)
• Integrate the new orca-libc to the CI and tooling workflow
• Later: integrate dawn to the CI and tooling workflow

[bitwitch]

Do we really want to let users override system orca dir? It seems like it could lead to using some version of the tool with another version of the source/libs.

Oh yeah probably not, this was an option from before that I left in. I will remove it.

Why is there source in release in addition to sources inside the tarball?

The two files Source code (zip) and Source code (tar.gz) are just there by default in github releases, it includes the entire repo. I don't know if these can be disabled but I can look, if we want to exclude them. The src dir inside each platform specific tarball is needed because it includes source files generated by building the runtime, like bindings, that don't exist in the repo source.

Some files have formatting that's a little inconsistent with our clang-format config, eg braces on the right, etc. and some have windows line endings (I never remember the git incantations for that, it's a pain...)

Ah yeah sorry about that. I'm not used to using clang so I didn't even think of using clang-format. I'll clean it up.

Thanks for the great notes, I will start looking into the rest of these.

[bitwitch]

What's the reason for the changes in ui.c? Did MSVC complain, or is it to avoid a buggy conversion (in the latter case should the macro be made more explicit wrt conversions)? Anyway we should probably split that out from this PR.

The commit in question is 4ff922b6de0d176c1b907c932358a5e112ec514a , @rdunnington do you remember the reason for the changes in ui.c here?

[rdunnington]

Yeah there are differences in how clang defines size_t on windows vs macOS, which was triggering errors in how we were dealing with it when passing size_t variables to the _Generic macro. The comment in the commit has some more details. I remember doing a few rounds of this until we finally landed on a solution that built on all platforms.

[martinfouilleul]

ah yeah I remember this one. Does this mean that the casts in ui.c were a temporary workaround to silence those errors, and can now be safely removed?

[rdunnington]

Hmm, maybe, I forget the exact details. We could probably try but would need to build a few scenarios:

• SDK and runtime on macOS
• SDK and runtime on Windows
• UI wasm sample on macOS
• UI wasm sample on Windows

[bvisness]

I see the tooling build still uses MSVC on Windows - can/should we switch to using clang? I feel like we talked about doing this before, since everyone will need clang for their wasm compiles anyway.

[martinfouilleul]

Yes, agreed

[bitwitch]

I see the tooling build still uses MSVC on Windows - can/should we switch to using clang? I feel like we talked about doing this before, since everyone will need clang for their wasm compiles anyway.

Oh I see. Well libcurl and zlib are both compiled using msvc (they both use nmake). zlib seems like it is relatively straightforward so it could probably be manually compiled with clang, but libcurl is not straightforward. I really don't know how to go about building it with clang.

[martinfouilleul]

Nice work! could you squash these into one (or a few) commits, also excluding ui.c? I think I'll merge that first, then we can continue work on the orca-libc branch to integrate the libc.

[martinfouilleul]

You also have some CRLF in here (eg at the beginning of bundle.c), what are your settings for git config core.autocrlf?

[martinfouilleul]

On macOS I also get this when trying to build the tool locally:

./osx_system.c:249:13: error: initializing 'oc_str8' (aka 'struct oc_str8') with an expression of incompatible type 'char *'
  249 |     oc_str8 path_cstr = oc_str8_to_cstring(a, path);
      |             ^           ~~~~~~~~~~~~~~~~~~~~~~~~~~~
./osx_system.c:250:21: error: passing 'oc_str8' (aka 'struct oc_str8') to parameter of incompatible type 'const char *'
  250 |         DIR *dir = opendir(path_cstr);
      |                            ^~~~~~~~~

[martinfouilleul]

A couple other things from trying to follow the user workflow on macOS:

• [x] The tooling does not seem to download libGLESv2.dylib and libEGL.dylib (so the bundle command fails when trying to copy that)
• [x] The examples build scripts should be updated to use zsh and to support SDK paths with spaces (I can take care of this one).
• [x] At first I downloaded orca-mac-x64.tar.gz instead of the CLI tool, and I think there's a risk of unnecessary confusion here, so maybe we should name this orca-sdk-mac-x64.tar.gz and the CLI tool orca-user-tooling-mac-x64.tar.gz or something?

[bitwitch]

Nice work! could you squash these into one (or a few) commits, also excluding ui.c? I think I'll merge that first, then we can continue work on the orca-libc branch to integrate the libc.

Alright I squished it. I excluded commit 4ff922b6de0d176c1b907c932358a5e112ec514a which had the changes to ui.c. Note that there were also some changes in that commit to src/util/macros.h.

[martinfouilleul]

Nice work! could you squash these into one (or a few) commits, also excluding ui.c? I think I'll merge that first, then we can continue work on the orca-libc branch to integrate the libc.

Alright I squished it. I excluded commit 4ff922b which had the changes to ui.c. Note that there were also some changes in that commit to src/util/macros.h.

Thanks, I cherry picked this commit and applied it on main separately.

[martinfouilleul]

Ok I'm going to merge this since we have enough of the workflows fleshed out and I want to start using the new tooling on the other fronts. Remaining work on the tooling can be done as additional incremental PRs.