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.

Sign up for GitHub

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

Jump to bottom

add defines at configure time for wayland protocol interface versions #13993

Closed
wants to merge 2 commits into from
Closed

add defines at configure time for wayland protocol interface versions #13993

wants to merge 2 commits into from

Conversation

Dudemanguy
Copy link
Member

Maybe this is overengineering it but it's kind of dumb to not have this information at configure time in the first place.

@Dudemanguy
build: add a wayland-interface-parse.py script
5531118 
In wayland, we have the annoying workflow where the protocol code is
generated at compile time from xml files and linked statically. This has
the assumption that the protocol version that is used to build mpv and
the protocol version that is used to build the compositor are the same.
They might not be which could lead to the compositor advertising support
for some wayland interface that mpv has code to use but the actual
linked protocol library doesn't support it.

Amazingly, wayland-scanner doesn't generate interface version defines in
the includes or anything useful like that (why?). So, give up and do it
ourselves during configure time and throw it in config.h.
@Dudemanguy
wayland: restrict xdg_wm_base to the supported client version
751def4 
With the previous commit, we now know what version of xdg_wm_base is
supported on the client side. Do the MPMIN check against that instead of
blindly on version 6. Fixes mpv-player#13986.
@github-actions GitHub Actions
Copy link

Download the artifacts for this pull request:

Windows
macOS

@na-na-hi
Copy link
Contributor

Is it not sufficient to check #if HAVE_WAYLAND_PROTOCOLS_1_XX to determine the max version supported? It is already done to check the existence of some other protocols.

@Dudemanguy
Copy link
Member Author

It is possible. But it doesn't solve the fundamental issue which is that we have no way of knowing interface versions at compile time. I think xdg_wm_base is the only thing that currently has this issue, but perhaps something like this would need to be done again in the future for some other protocol.

But again, it's kind of overengineering like I said.

@na-na-hi
Copy link
Contributor

na-na-hi commented Apr 26, 2024 •
edited

But it doesn't solve the fundamental issue which is that we have no way of knowing interface versions at compile time.

It is possible by calculating the maximum value of all interested SINCE_VERSION macros, and limit wl_registry_bind to use that max value. If something isn't defined, don't include it in the calculation. In the mentioned issue, if XDG_TOPLEVEL_WM_CAPABILITIES_SINCE_VERSION (5) isn't defined, the calculated max protocol version will be less than 5.

@Dudemanguy
Copy link
Member Author

It is possible by calculating the maximum value of all interested SINCE_VERSION macros, and limit wl_registry_bind to use that max value.

OK maybe I should have typed "sane way" to be more accurate. I would rather this or hack around in HAVE_WAYLAND_PROTOCOLS than do that.

@ruihe774 ruihe774 mentioned this pull request Apr 26, 2024
Closed
@ruihe774
Copy link
Contributor

FWIW, I'd prefer using SINCE_VERSION instead of manually parsing the interface XML files again, giving they are already parsed by wayland-scanner to generate the header files.

@llyyr
Copy link
Contributor

llyyr commented Apr 26, 2024

Maybe you could report it to the wayland-protos issue tracker so at least there's a chance we could get rid of this in the future?

@Dudemanguy
Copy link
Member Author

I already have an open MR for wayland-scanner that adds this version information.

kasper93
kasper93 approved these changes May 5, 2024
View reviewed changes
Copy link
Contributor

@kasper93 kasper93 left a comment •
edited

Choose a reason for hiding this comment

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

looks ok, except one nitpick

TOOLS/wayland-interface-parse.py
for line in f:
if "interface name=" in line:
interfaces.append(parse_interface(line))
sys.stdout.write(','.join(interfaces))
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, but there is mix of " and ' usage, make it use one of them.

@Dudemanguy
Copy link
Member Author

I'm waiting for a response from upstream before I decide what to do with this one.

@Dudemanguy
Copy link
Member Author

We'll just hack around it for now. Hopefully upstream will be receptive to making scanner more useful later on.

@Dudemanguy Dudemanguy closed this May 16, 2024
@Dudemanguy Dudemanguy deleted the wayland-interface-parsing branch May 16, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kasper93 kasper93 kasper93 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Dudemanguy @na-na-hi @ruihe774 @llyyr @kasper93