Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update go-gl/glfw for glfw 3.3.8 upgrade #3131

Merged
merged 4 commits into from
Oct 22, 2022
Merged

Conversation

Jacalz
Copy link
Member

@Jacalz Jacalz commented Jul 10, 2022

Description:

This brings in a bunch of nice bug fixes (and more Wayland APIs exposed through the bindings). See https://www.glfw.org/changelog.html for more information.

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

  • Updated the vendor folder (using go mod vendor).

@andydotxyz
Copy link
Member

Thanks for doing this.
rebase this to latest develop should fix the tests.

@Jacalz
Copy link
Member Author

Jacalz commented Jul 11, 2022

It looks like there are some Wayland build issues. Will have to look into that.

@Jacalz
Copy link
Member Author

Jacalz commented Jul 11, 2022

I honestly don't understand why the Wayland build is failing. It works locally on my Fedora 35 install 🤔

@Bluebugs
Copy link
Contributor

The Wayland issue is due to a mismatch between the wayland scanner used to generate the header in glfw package and the host wayland version. I have no idea on how to fix it, as the expectation is to run the wayland-scanner as part of glfw packaging.

@Bluebugs
Copy link
Contributor

Wayland upstream does recommend to actually not bundle those generated file, but build them as part of the host build step. In our case where we have a base version support and because Wayland is backward compatible, we should generate those file on the oldest Linux with Wayland support. It is not ideal, but otherwise we might have those build issue again (other side effect of that strategy is that we would distribute a file that do not leverage fixes on more recent system).

@Jacalz
Copy link
Member Author

Jacalz commented Jul 11, 2022

Ah, I see. Thanks @Bluebugs. I think I might know how to fix that upstream. Will look at that later today or possible tomorrow :)

@Jacalz
Copy link
Member Author

Jacalz commented Jul 12, 2022

The protocols are now regenerated upstream. Let's hope that the compile issues go away now.

@Jacalz
Copy link
Member Author

Jacalz commented Jul 12, 2022

Nope. Still results in issues. I'm a bit lost on why they appear. Running fyne_demo locally works just fine with -tags wayland. Running the tests locally with it seems to hang. Need to give it a new test when I have more time.

@Bluebugs
Copy link
Contributor

Ubuntu 20.04 does use Wayland 1.18.0 not 1.20.

@Jacalz
Copy link
Member Author

Jacalz commented Jul 13, 2022

Oh no. That will likely stall this a while. I don’t have Ubuntu set up and probably won’t have the time to do so in the near future.

@andydotxyz
Copy link
Member

It would seem unwise to depend on something that new. Is it a build requirement or just a code-generation requirement?

@Bluebugs
Copy link
Contributor

It would seem unwise to depend on something that new. Is it a build requirement or just a code-generation requirement?

My understanding is that an old version of the Wayland header and the binding that goes with it should be ABI compatible with any more recent version (They are serious about there API). I do not know if glfw and go-glfw are as good at it and properly detect version to not depend on newer version.

@Jacalz
Copy link
Member Author

Jacalz commented Aug 11, 2022

TODO for myself: Remember to upgrade to 3.3.8 and update glfw-js as well once I have regenerated the files on an older version of Ubuntu.

@Jacalz Jacalz changed the title Update go-gl/glfw for glfw 3.3.7 upgrade Update go-gl/glfw for glfw 3.3.8 upgrade Aug 27, 2022
@Jacalz
Copy link
Member Author

Jacalz commented Sep 29, 2022

I'm sorry that this has become stagnant. My workstation crashes during high memory usage due to a faulty DDR4 ram stick and I tried to install Ubuntu in a VM but it did not work.

This will be blocked until I can get my hands on a set of DDR4 memory sticks for a good price :/

@Jacalz
Copy link
Member Author

Jacalz commented Oct 17, 2022

This should now be good to go. Managed to rebuild the Wayland protocols on Ubuntu 20.04 and the changes have been merged upstream. Tests should now pass, I hope :)

@coveralls
Copy link

coveralls commented Oct 17, 2022

Coverage Status

Coverage remained the same at 61.356% when pulling c9d6791 on Jacalz:glfw into 78f6dd9 on fyne-io:develop.

@Jacalz Jacalz requested a review from andydotxyz October 17, 2022 17:01
@andydotxyz
Copy link
Member

I attempted building this branch with -tags wayland but get the following:

# github.com/go-gl/glfw/v3.3/glfw
In file included from ../../vendor/github.com/go-gl/glfw/v3.3/glfw/c_glfw_lin.go:9:
../../vendor/github.com/go-gl/glfw/v3.3/glfw/glfw/src/wl_window.c: In function 'createTmpfileCloexec':
../../vendor/github.com/go-gl/glfw/v3.3/glfw/glfw/src/wl_window.c:53:10: warning: implicit declaration of function 'mkostemp'; did you mean 'mkstemp'? [-Wimplicit-function-declaration]
   53 |     fd = mkostemp(tmpname, O_CLOEXEC);
      |          ^~~~~~~~
      |          mkstemp
../../vendor/github.com/go-gl/glfw/v3.3/glfw/glfw/src/wl_window.c: In function 'waitForData':
../../vendor/github.com/go-gl/glfw/v3.3/glfw/glfw/src/wl_window.c:204:32: warning: implicit declaration of function 'ppoll'; did you mean 'poll'? [-Wimplicit-function-declaration]
  204 |             const int result = ppoll(fds, count, &ts, NULL);
      |                                ^~~~~
      |                                poll
../../vendor/github.com/go-gl/glfw/v3.3/glfw/glfw/src/wl_window.c: In function 'readDataOfferAsString':
../../vendor/github.com/go-gl/glfw/v3.3/glfw/glfw/src/wl_window.c:953:9: warning: implicit declaration of function 'pipe2'; did you mean 'pipe'? [-Wimplicit-function-declaration]
  953 |     if (pipe2(fds, O_CLOEXEC) == -1)
      |         ^~~~~
      |         pipe

@Jacalz
Copy link
Member Author

Jacalz commented Oct 18, 2022

They are just warnings. The compilation still works though. The warnings are tracked with go-gl/glfw#359 upstream.

@Bluebugs
Copy link
Contributor

Bluebugs commented Oct 18, 2022

I do have the same warning as everyone, but it does compile a binary. I do not have a Wayland setup to test the result. I am on Arch Linux with wayland-scanner and friends being 1.21.0..

@andydotxyz
Copy link
Member

They are just warnings. The compilation still works though. The warnings are tracked with go-gl/glfw#359 upstream.

My apologies you are quite right - I'm not sure why I thought it was not working. Tested and the resulting executable runs great!

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to be up to date, thanks.
Does it fix any registered issues?

@Jacalz
Copy link
Member Author

Jacalz commented Oct 22, 2022

Good to be up to date, thanks.

Does it fix any registered issues?

Good question. There really are a lot of fixes but nothing that I know should fix specific open issues on our end. I guess we'll see sooner or later 😅

@Jacalz Jacalz merged commit 939f8ab into fyne-io:develop Oct 22, 2022
@Jacalz Jacalz deleted the glfw branch October 22, 2022 06:41
@andydotxyz andydotxyz mentioned this pull request Oct 31, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants