Consider moving 3DTiles/glTF class generation to CI

[csciguy8]

We generate the classes manually and have documentation as of PR #705 .

Could we benefit from having our build do this for us? (and by extension, continuous integration)

Some initial thoughts:

(+) Don't have to manually generate these classes ever again
(+) Don't have to commit the generated classes
(+) Ensures our generation code always works (and is up to date)
(-) More dependencies for cesium-native builds
(-) Slower cesium-native builds (not sure how much)

[timoore]

I applaud the sentiment to generate these classes during the build. Speaking selfishly, it would be a drag to have more dependencies in the cesium-native build.

[kring]

Thanks @timoore. Just to clarify a few points:

• The dependency we're talking about is Node.js, which is use to run the code generator.
• Currently the generated files are checked into the repo, so you only need Node.js if you want to regenerate the code, which isn't too common (the 3D Tiles and glTF specs change slowly).
• This issue is suggesting that we not check the generated files into the repo. Instead, we would run the generator as part of the build process. Which means that building cesium-native would require Node.js.

So if I understand your comment correctly, you're saying that you would prefer that we leave things as they are in order to avoid adding a new build-time dependency on Node.js. Do I have that right?

[timoore]

That's a good summary of what I thought was proposed. Node.js is probably not a big deal for Cesium folks, but it would definitely be a surprising dependency in the VSG world. For Unreal / Unity users, would that dependency be sucked in automatically during the build?

[kring]

For Unreal / Unity users, would that dependency be sucked in automatically during the build?

No, it would probably just be a prerequisite. But Cesium for Unreal and Cesium for Unity users don't usually build cesium-native themselves, they just use pre-built binaries. So it's only people developing these plugins that would need to have Node.js.

In any case, I think you're convincing me that we might just want to stick with the status quo. The main motivation was to avoid having PRs with changes to a hundred generated files that we have to skip over when reviewing (and maybe verify that the committed files reflect what was generated, otherwise we'll lose any modifications next time we regenerate). Curious what @csciguy8 thinks.

[csciguy8]

I applaud the sentiment to generate these classes during the build. Speaking selfishly, it would be a drag to have more dependencies in the cesium-native build.

Well technically, Node.js is already a build dependency, whether it's something that's on CI or not. Our builds need the generated classes. The generation process needs Node.js. Generating the classes needs time and a machine to run on.

As it stands from a Devops perspective, we have maintenance issues:

• When do these classes need to be generated? (how do we know they are out of sync)
• Who is building these classes? (from this PR, @kring 's local machine)
• For the machine that built these classes, is the generation build setup documented?
• How do we verify the classes are correct? Does the reviewer have the same setup and repeatability as the machine that committed them?
• Are future contributors discouraged from changing related code because of this complexity?

Don't get me wrong, I understand the motivation to keep our local builds as fast and simple as possible. I like this too. And as stated before, the scope of this would only be for those building cesium-native. And who knows, maybe it's possible to move the generation process to another repo and keep this out of cesium-native altogether?

But if we encounter these questions over and over in future PRs, it may actually be slowing us down by keeping this process manual.

[kring]

I see what you're saying, but I don't think I can get worked up about it. They get regenerated when we see a need to do so because the spec or the generator changed, and having it being an intentional step to pick up spec changes isn't necessarily a bad thing. Anyone can build the classes, and it's quite repeatable, and there are even instructions for doing it in the README now. We can verify they're correct by re-running the generator itself and verifying there are no resulting changes (It would of course be concerning if differences of setup ever caused the generated code to be different, but that has not happened so far and I think it's reasonably unlikely to ever happen in the future). And of course we also verify by compiling cesium-native and running the test suite. I don't think the need to install Node.js and run a couple of commands should be a major impediment to changing this code for anyone that needs to.

We could get the best of both worlds with a github action or somesuch that periodically re-runs the generator from the latest spec and opens a PR if there are any changes (kind of like dependabot). I think it's hard to make a case that that's among the best things we can spend our time on right now, though.

[csciguy8]

I see what you're saying, but I don't think I can get worked up about it....

We could get the best of both worlds with a github action or somesuch that periodically re-runs the generator from the latest spec and opens a PR if there are any changes (kind of like dependabot). I think it's hard to make a case that that's among the best things we can spend our time on right now, though.

Agreed. This does feel like a separate process, with ideally a separate workflow.

I don't believe there's any immediacy to this, and I'm not worked up about it either. We can do it when we feel it it has the right value at the right time, just like any other issue.

[csciguy8]

Closing this issue.

It's been considered, and not something concerning enough to leave open.