Add glad library for glfw.

[cloudwebrtc]

#9822 Depends on this patch.

[cloudwebrtc]

@stuartmorgan Please review the dependency patch for #9822

[cbracken]

@cloudwebrtc are the C headers/sources here being copied from another repo?

If so, the way we add third-party dependencies is via an entry in the DEPS file in the engine that clones this to src/third_party/glad and add a BUILD.gn file in the buildroot. Typically, this also invovles us setting up an auto-roller that clones a copy of the upstream sources to a git instance hosted somewhere on googlesource.com (we can set that up for you if these sources exist somewhere online already and you can point us to them).

[stuartmorgan-g]

I believe these are generated, rather than something we could pull with DEPS. But in that case we need a readme in the folder that clearly explains where it came from, how it's generated, how to update it, etc.

[cloudwebrtc]

@cbracken @stuartmorgan The glad sources is actually included in the GLFW repo, the original file is from the third_party/glfw repository of fuchsia, so before I Modify BUILD.gn directly in a PR of buildtools to create a target for glad.

I think there are two options here:

1, If we don't want a duplicate file in flutter/engine, we can modify buildroot/build/secondary/third_party/glfw/BUILD.gn to add a glad target, because glad.h/glad.c files already exist.

I looked at the new version of fuchsia/third_party/glfw repo, which also contains the glad file. The latest version of glfw separates glad.h from the gl and vulkan versions. I checked the glad/gl.h The necessary function is compatible with the current version.

2, Copy the glad source from the GLFW repository, create a third-party library separately, add a separate BUILD.gn, readme, upgrade instructions. When you need to upgrade, we need to copy again from fuchsia/third_party/glfw repo or use https://glad.dav1d.de to generate glad source code.

I personally prefer the first one, create a glad target to buildroot/build/secondary/third_party/glfw/BUILD.gn, because there is no need to add any source code, and it will not increase the upgrade workload.

[stuartmorgan-g]

1, If we don't want a duplicate file in flutter/engine, we can modify buildroot/build/secondary/third_party/glfw/BUILD.gn to add a glad target, because glad.h/glad.c files already exist.

As I explained before, I don't this this is a good idea. From what I can see glad is not conceptually part of GLFW, but a totally different project. The fact that some GLFW example code happens to also use glad doesn't mean that we should use that copy; our third-party dependencies should be explicit, not included transitively from the third-party dependencies of our third-party dependencies.

2, Copy the glad source from the GLFW repository, create a third-party library separately, add a separate BUILD.gn, readme, upgrade instructions. When you need to upgrade, we need to copy again from fuchsia/third_party/glfw repo

This doesn't seem conceptually much different from the above, although at least it makes the dependencies explicit.

use https://glad.dav1d.de to generate glad source code.

If that's the authoritative source, then we should do that. I thought that's what I had suggested before, and assumed is what this patch was.

I personally prefer the first one, create a glad target to buildroot/build/secondary/third_party/glfw/BUILD.gn, because there is no need to add any source code

It's not clear to me why this is a goal.

and it will not increase the upgrade workload.

Assuming that the version generated for use in a GLFW example app stays compatible with our needs.

[cloudwebrtc]

It seems that https://gen.glad.sh is the authoritative source, I found it in the glfw official source file and has a link with parameters.

https://gen.glad.sh/#api=gl%3Acompatibility%3D3.3&extensions=GL_ARB_multisample%2CGL_ARB_robustness%2CGL_KHR_debug&generator=c&options=

In the case where the gl version number is fixed, the generated file does not seem to change.

[cloudwebrtc]

@stuartmorgan I created a README.md, please review.
Also, I tried to create the generated source code from https://gen.glad.sh, but the gladLoadGLLoader function will be missing, which will cause compilation errors, so follow the https://fuchsia.googlesource.com/third_party/glfw/deps version upgrade possible Will be more reliable.

[stuartmorgan-g]

I'm not comfortable with checking in a file that we don't apparently understand how to create from the authoritative source, and thus don't know how to update ourselves if we needed to update it.

The version of this file on master contains the exact details of constructing it from the source, which seems far better. I'm not clear on why we can't do the same thing here instead of relying on this older copy of unclear origin.

[cloudwebrtc]

Sorry, I have always thought that the glad version must be with glfw. After I tested that the glad file from http://glad.sh can still work fine, I proved that I was wrong, glfw and glad are working independently. So, you said that generating glad from an authoritative source is correct. I am working on the engine's texture rendering test and will update the PR later.

[cloudwebrtc]

@stuartmorgan PR updated, please review.

[stuartmorgan-g]

A few more questions but this looks structurally like what we want now.

Also I think you need to merge in the current master to resolve the CI errors.

[cloudwebrtc]

@stuartmorgan All done, please review again.

[cloudwebrtc]

@stuartmorgan Updated, please review again.

[stuartmorgan-g]

@cbracken Do we want a mirror of this? It seems like we have other direct GitHub dependencies, but I'm not sure if there's a policy for when that's allowed.

[cloudwebrtc]

I changed it to run glad.__main__.py directly in generate.py.

[stuartmorgan-g]

I can do a full re-review later, but from a quick look:

and whatever it uses internally as inputs

This part is still missing. The action should have, as inputs, all the files that, were they to change, would require re-running this step. That means all the relevant py files, and from a brief glance it looks like there are some .xml files that are probably relevant as well. Without that, the step will never re-run if the outputs exist, even if glad is updated to a newer version, which will cause hard-to-find issues for some people who have stale generated files.

[cloudwebrtc]

I can do a full re-review later, but from a quick look:

and whatever it uses internally as inputs

This part is still missing. The action should have, as inputs, all the files that, were they to change, would require re-running this step. That means all the relevant py files, and from a brief glance it looks like there are some .xml files that are probably relevant as well. Without that, the step will never re-run if the outputs exist, even if glad is updated to a newer version, which will cause hard-to-find issues for some people who have stale generated files.

I don’t know much about how the action works, you mean scripts/glad/files/gl.xml as inputs to the action?
If we update the Dav1dde/glad repository, the action will detect the xml file, and if there is a change, the action will be re-executed.

[stuartmorgan-g]

An action runs when any of its inputs are newer than the outputs. For incremental build to work reliably you need to list every file that, were it to change, should cause the script to run again.

As a said above, that is all of the python files used by the script, as well as any data files.

[cloudwebrtc]

I need to use all the following files as inputs to the action. Is this correct?
glad/*.py
glad/files/*.*
glad/lang/*.*

This is a list of files for the https://github.com/Dav1dde/glad/tree/master/glad repository.

[stuartmorgan-g]

You need to list every file that affects the output of the script. Whether or not that's every file in the repository I don't know; you'll need to look at the details of the script.

[cloudwebrtc]

Thanks, I can try to do it.

[cbracken]

@cloudwebrtc any luck with the new approach?

[cloudwebrtc]

@cbracken Sorry for the late update, @stuartmorgan I have added the relevant files to inputs and tested them. Modifying any .py file can trigger action again.

[cloudwebrtc]

@stuartmorgan Please review again.

[stuartmorgan-g]

The license script is still unhappy about the new files in the Flutter part of the tree that have different licenses. @cbracken @Hixie is the right approach here to point DEPS to the buildroot's third-party (as we usually do for DEPS) and put our added wrapper script somewhere not in third_party like build?

[stuartmorgan-g]

Adding @cbracken as a reviewer for the question above.

[stuartmorgan-g]

Closing per #9822 (comment). We can revisit if we end up needing GLFW texture support.