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

[RFC] wayland: vendor wayland protocols #14172

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mahkoh
Copy link
Contributor

@mahkoh mahkoh commented May 18, 2024

The interesting stuff happens in the second commit.

The first commit only copies the xml files into the project. Instead, it might be better to use git submodules that can be updated easily. But mpv does not currently use submodules so I didn't want to start doing that.

Copy link

github-actions bot commented May 18, 2024

Download the artifacts for this pull request:

Windows
macOS

@ruihe774
Copy link
Contributor

da713ec LGTM.

For adf539f, I'd rather see a git submodule than see ~7000 lines of addition. This is only my personal opinion.

@Dudemanguy I wonder whether you've been convinced by guys at wayland-scanner#389. I am personally not against their idea.

cc @llyyr @na-na-hi

@na-na-hi
Copy link
Contributor

I think it's better to only include the protocols we want and only update when necessary, and don't use submodule which voids the whole point of vendoring. This is the approach that SDL uses.

@ruihe774
Copy link
Contributor

ruihe774 commented May 19, 2024

I think it is acceptable if the curated interface files are less than ~500 lines.

Otherwise, maybe an alternative solution is to put and maintain the interface XML files in a separated repo.

@Dudemanguy
Copy link
Member

I don't think the extra maintenance effort is worth it.

@mahkoh
Copy link
Contributor Author

mahkoh commented May 19, 2024

I think it is acceptable if the curated interface files are less than ~500 lines.

This MR contains only those protocols that are being used by mpv.

@kasper93
Copy link
Contributor

kasper93 commented May 19, 2024

submodule which voids the whole point of vendoring

Why? It is just another way of delivering files, that would otherwise be copied into mpv repository. For me it is unacceptable to ship almost 8k loc of external xml data, that needs to be periodically updated.

dependency('wayland-protocols', version: '>= 1.25', required: get_option('wayland')),

If the problem is with the version, just update the required version to 1.35, it is not our job to distribute those files. And with meson it is as simple as meson wrap install wayland-protocols to allow it to download those xmls if required version is not available in your distro.

@mahkoh
Copy link
Contributor Author

mahkoh commented May 19, 2024

For me it is unacceptable to ship almost 8k loc of external xml data, that needs to be periodically updated.

There is no need to update the files unless you want to use new functionality added in subsequent protocol versions. And at that point you only have to update the protocol file whose version you want to raise.

@kasper93
Copy link
Contributor

There is no need to update the files

If there was no need to update the files, we wouldn't need this PR or #ifdefs in the code, don't we? Eventually they will be updated, but this is beside the point, I don't think it is good idea to include them, there are clearly better solutions.

@mahkoh
Copy link
Contributor Author

mahkoh commented May 19, 2024

Eventually they will be updated

Sure but this is just part of normal feature development. If let's say mpv added support for fractional scaling today, then you would have to 1) copy the fractional scaling xml file into the repo and 2) implement the protocol. The burden of copying the file is negligible compared to the actual development work.

There is also little chance of getting it wrong since your code will not compile if you're trying to use a new event or new request from a protocol version that is newer than the checked-in xml file.

I don't think it is good idea to include them, there are clearly better solutions.

Of course. The main goal is to get rid of the conditional compilation.

Signed-off-by: Julian Orth <ju.orth@gmail.com>
Signed-off-by: Julian Orth <ju.orth@gmail.com>
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.

None yet

5 participants