C tooling #55

bitwitch · 2024-02-18T16:03:02Z

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 · 2024-02-18T17:35:46Z

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 · 2024-02-18T18:31:20Z

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 · 2024-02-18T21:04:30Z

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 4ff922b , @rdunnington do you remember the reason for the changes in ui.c here?

rdunnington · 2024-02-19T04:41:07Z

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 · 2024-02-19T08:10:30Z

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 · 2024-02-19T16:49:49Z

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 · 2024-02-19T17:07:53Z

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 · 2024-02-19T17:18:56Z

Yes, agreed

bitwitch · 2024-02-20T05:56:18Z

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 · 2024-03-03T18:19:03Z

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 · 2024-03-03T18:23:36Z

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

martinfouilleul · 2024-03-03T18:36:03Z

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 · 2024-03-03T20:44:04Z

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

The tooling does not seem to download libGLESv2.dylib and libEGL.dylib (so the bundle command fails when trying to copy that)
The examples build scripts should be updated to use zsh and to support SDK paths with spaces (I can take care of this one).
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?

This is mostly Ben's work splitting dev and user tooling, and beginning to rewrite the user tooling in C.

It now uses GetFullPathNameW and dynamically allocates a path string

bitwitch · 2024-03-06T04:35:09Z

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.

* Creating releases in CI * Dev Tooling: * `orcadev install` - install a dev build in the system orca dir * User Tooling: * `orca update` - download and install latest version of orca * `orca install-path` - print system orca dir * `orca sdk-path --version` - print path to specific sdk version * `orca bundle --version` - bundle with a specific sdk version * `orca list` - list installed versions

* support size_t explicitly for non-windows platforms * added casts in a couple places to ensure oc_clamp() deals with the same types (Cherry-picked from 4ff922b, see #55 (comment)) Co-authored-by: Reuben Dunnington <reuben.dunnington@gmail.com> Co-authored-by: Martin Fouilleul <martinfouilleul@gmail.com>

martinfouilleul · 2024-03-10T21:58:22Z

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.

martinfouilleul self-assigned this Feb 18, 2024

martinfouilleul marked this pull request as draft February 18, 2024 16:56

bitwitch force-pushed the c-tooling branch 2 times, most recently from 46ebf52 to 1c094e8 Compare February 27, 2024 20:08

bvisness and others added 2 commits March 5, 2024 23:02

tooling: building a foundation for the new tooling

03d9447

This is mostly Ben's work splitting dev and user tooling, and beginning to rewrite the user tooling in C.

oc_path_canonical on windows no longer relies on MAX_PATH

7d6c573

It now uses GetFullPathNameW and dynamically allocates a path string

bitwitch force-pushed the c-tooling branch from 405f57e to 3c6653b Compare March 6, 2024 04:25

bitwitch force-pushed the c-tooling branch 3 times, most recently from c46636a to f3ecbe5 Compare March 6, 2024 07:14

bitwitch and others added 4 commits March 6, 2024 02:22

display custom icon in win32 window

e111146

add build-dawn.sh

7911108

Fix copy path in build-dawn.sh

90cdbdb

bitwitch force-pushed the c-tooling branch from f3ecbe5 to 90cdbdb Compare March 6, 2024 07:22

martinfouilleul added 3 commits March 6, 2024 23:47

update macOS sample build scripts

0e69026

Build Orca wasm SDK as part of the runtime-build command

7f77e79

fix dev.py to explicitly use brew-installed clang on macOS

118fd56

martinfouilleul force-pushed the c-tooling branch from 72ee334 to 118fd56 Compare March 7, 2024 18:56

martinfouilleul added 10 commits March 7, 2024 20:32

add comment about CI in dev.py

b2593db

use github m1 runners

918cd53

package liborca_wasm.sdk

0c5ef9b

update mac example build scripts

264b629

fix update replace and exec tool, for when tool path contains spaces

a729c9f

allow passing an install dir to

1c42122

merge orca-libc

e59c2b0

fix dev and clock build scripts

c564771

update windows build scripts

026d574

martinfouilleul marked this pull request as ready for review March 10, 2024 19:03

martinfouilleul force-pushed the c-tooling branch from baac14f to 1608e7e Compare March 10, 2024 21:46

fix compiling libc to use correct clang in CI

1626655

martinfouilleul force-pushed the c-tooling branch from 1608e7e to 1626655 Compare March 10, 2024 21:49

martinfouilleul merged commit 8647cf2 into main Mar 10, 2024

martinfouilleul deleted the c-tooling branch March 10, 2024 22:00

This was referenced Mar 11, 2024

Separate dev tooling and user tooling #22

Closed

Build a better workflow around the system install directory #23

Closed

Distribute premade builds of user tooling #24

Closed

This was referenced Apr 7, 2024

Tooling roadmap #26

Closed

Win32 app icon #2

Closed

Pre-compile wasm SDK #1

Closed

Installing and vendoring has so many problems #16

Closed

Windows PATH updates can leave quotes in your PATH (and duplicates) #13

Closed

orca help is unhelpful #15

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

C tooling #55

C tooling #55

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

C tooling #55

C tooling #55

Uh oh!

Conversation

Uh oh!

Tool

CI

Misc

TODOs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!