Add glad library for glfw. by cloudwebrtc · Pull Request #12265 · flutter-team-archive/engine

cloudwebrtc · 2019-09-13T16:02:54Z

#9822 Depends on this patch.

Merge pull request from flutter/engine

cloudwebrtc · 2019-09-13T16:08:54Z

@stuartmorgan Please review the dependency patch for #9822

cbracken · 2019-09-16T18:19:52Z

@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 · 2019-09-16T23:33:07Z

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 · 2019-09-17T02:37:55Z

@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 · 2019-09-17T04:50:05Z

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 · 2019-09-17T12:19:02Z

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 · 2019-09-17T14:45:59Z

@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 · 2019-09-23T17:08:23Z

+The `GLAD` library is used for the texture of glfw, which is used to create textures to render RGBA data.
+
+# Notice
+The current `glad` source is a copy from `fuchsia/third_party/glfw/deps` and the first version is copied from https://fuchsia.googlesource.com/third_party/glfw/+/999f3556fdd80983b10051746264489f2cb1ef16/deps.


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.

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 · 2019-09-24T02:31:55Z

@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 · 2019-09-27T01:16:40Z

@stuartmorgan All done, please review again.

cloudwebrtc · 2019-10-10T15:23:40Z

@stuartmorgan Updated, please review again.

stuartmorgan-g · 2019-10-11T19:00:37Z

   Var('dart_git') + '/when.git' + '@' + '0.2.0',

+  'src/flutter/third_party/glad/scripts':
+   Var('github_git') + '/Dav1dde/glad.git' + '@' + 'de6c39e3040c987323b8ed078c36442f4fb681b3',


@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 · 2019-10-12T15:56:32Z

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

stuartmorgan-g · 2019-10-12T16:43:33Z

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 · 2019-10-15T09:43:12Z

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 · 2019-10-15T14:23:51Z

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 · 2019-10-15T14:57:25Z

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 · 2019-10-15T15:22:08Z

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 · 2019-10-15T16:07:28Z

Thanks, I can try to do it.

cbracken · 2019-11-04T18:38:13Z

@cloudwebrtc any luck with the new approach?

cloudwebrtc · 2019-11-07T03:54:07Z

@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 · 2019-11-14T15:31:24Z

@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 · 2019-12-08T21:59:48Z

Adding @cbracken as a reviewer for the question above.

stuartmorgan-g · 2020-06-17T22:56:08Z

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

cloudwebrtc added 6 commits March 29, 2019 00:15

Merge pull request #1 from flutter/master

5302cf8

Merge pull request from flutter/engine

Merge remote-tracking branch 'upstream/master'

2953e13

Merge remote-tracking branch 'upstream/master'

42b7502

Merge remote-tracking branch 'upstream/master'

ee2a336

Merge remote-tracking branch 'upstream/master'

bfd31aa

Add glad project.

7df876c

googlebot added cla: yes labels Sep 13, 2019

cbracken assigned stuartmorgan-g Sep 16, 2019

Create README.md

973c111

cloudwebrtc added 3 commits September 18, 2019 07:54

Update README.md

00ab503

clang-format.

e294632

clang-format.

40af09c

stuartmorgan-g reviewed Sep 23, 2019

View reviewed changes

cloudwebrtc added 2 commits September 24, 2019 10:28

Update.

561e3af

clang-format.

f62b6b6

cloudwebrtc mentioned this pull request Sep 24, 2019

Texture support for glfw. #9822

Closed

stuartmorgan-g reviewed Sep 26, 2019

View reviewed changes

Comment thread third_party/glad/BUILD.gn

Comment thread third_party/glad/BUILD.gn Outdated

Comment thread third_party/glad/BUILD.gn

Comment thread third_party/glad/README.md Outdated

Comment thread third_party/glad/README.md Outdated

Comment thread third_party/glad/README.md Outdated

cloudwebrtc added 3 commits September 27, 2019 09:06

Merge remote-tracking branch 'upstream/master' into glad

8d21caa

Update.

79c2e01

Update copyright block.

3bfe653

cloudwebrtc added 4 commits October 10, 2019 23:03

update.

e3cd53f

update.

8107ad8

Add arguments for generate.py.

b9c5c52

update.

f06c1be

cloudwebrtc changed the title ~~Add glad for glfw.~~ Add glad library. Oct 10, 2019

cloudwebrtc changed the title ~~Add glad library.~~ Add glad library for glfw. Oct 10, 2019

stuartmorgan-g suggested changes Oct 11, 2019

View reviewed changes

update.

a5d8dfe

cloudwebrtc mentioned this pull request Oct 15, 2019

Support Texture widget in GLFW embedding flutter/flutter#30718

Open

Add related files to inputs.

2467525

stuartmorgan-g suggested changes Nov 7, 2019

View reviewed changes

Comment thread third_party/glad/BUILD.gn

Comment thread third_party/glad/BUILD.gn Outdated

Comment thread third_party/glad/BUILD.gn Outdated

Comment thread third_party/glad/BUILD.gn Outdated

Comment thread third_party/glad/generate.py

Comment thread third_party/glad/LICENSE

cloudwebrtc added 2 commits November 14, 2019 23:12

update.

59c734b

Add comment for generate.py.

056a1c7

stuartmorgan-g reviewed Nov 17, 2019

View reviewed changes

Comment thread third_party/glad/BUILD.gn Outdated

update.

7d84e72

stuartmorgan-g requested a review from cbracken December 8, 2019 21:59

stuartmorgan-g closed this Jun 17, 2020

Uh oh!

Conversation

cloudwebrtc commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloudwebrtc commented Sep 13, 2019

Uh oh!

cbracken commented Sep 16, 2019

Uh oh!

stuartmorgan-g commented Sep 16, 2019

Uh oh!

cloudwebrtc commented Sep 17, 2019

Uh oh!

stuartmorgan-g commented Sep 17, 2019

Uh oh!

cloudwebrtc commented Sep 17, 2019

Uh oh!

cloudwebrtc commented Sep 17, 2019

Uh oh!

stuartmorgan-g Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

cloudwebrtc Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

cloudwebrtc commented Sep 24, 2019

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloudwebrtc commented Sep 27, 2019

Uh oh!

cloudwebrtc commented Oct 10, 2019

Uh oh!

Uh oh!

stuartmorgan-g Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cloudwebrtc commented Oct 12, 2019

Uh oh!

stuartmorgan-g commented Oct 12, 2019

Uh oh!

cloudwebrtc commented Oct 15, 2019

Uh oh!

stuartmorgan-g commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloudwebrtc commented Oct 15, 2019

Uh oh!

stuartmorgan-g commented Oct 15, 2019

Uh oh!

cloudwebrtc commented Oct 15, 2019

Uh oh!

cbracken commented Nov 4, 2019

Uh oh!

cloudwebrtc commented Nov 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloudwebrtc commented Nov 14, 2019

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g commented Dec 8, 2019

Uh oh!

stuartmorgan-g commented Jun 17, 2020

Uh oh!

cloudwebrtc commented Sep 13, 2019 •

edited

Loading

stuartmorgan-g commented Oct 15, 2019 •

edited

Loading