Skip to content

[sdl2] Re-enable ibus in build#29607

Merged
JavierMatosD merged 2 commits intomicrosoft:masterfrom
space-wizards:sdl2-ibus
Feb 13, 2023
Merged

[sdl2] Re-enable ibus in build#29607
JavierMatosD merged 2 commits intomicrosoft:masterfrom
space-wizards:sdl2-ibus

Conversation

@PJB3005
Copy link
Contributor

@PJB3005 PJB3005 commented Feb 12, 2023

This was disabled in #14275 because it caused the relevant libraries (ibus, glib, ...) to be depended on transitively.

Nowadays, SDL2 loads these completely at runtime, only needing the headers and such at compile time. It should be safe to re-enable this again.

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@PJB3005
Copy link
Contributor Author

PJB3005 commented Feb 12, 2023

@microsoft-github-policy-service agree

@dg0yt
Copy link
Contributor

dg0yt commented Feb 12, 2023

This change isn't sufficient.

  • sdl2 needs ibus headers to actually enable the feature. However, there is no ibus port yet. This means you would need to rely on system packages.
  • If ibus is meant to be enabled, it must actually be enabled. The capabilities of the binary artifacts must not change depending on the presence of system packages. This may need modification to sdl2's build system.
  • IIUC ibus needs dbus, and maybe additional packages. At least dbus is a port in vcpkg.

I wouldn't say system package dependencies are unacceptable, but at least they should be limited and scoped by an optional feature. And the user should be made aware of it.

@PJB3005
Copy link
Contributor Author

PJB3005 commented Feb 12, 2023

I'm fine with introducing some warning messages like the package currently does for Wayland/X11, but please understand that this is only bringing the ibus system up to par with the rest of the port. Many of the current features of SDL2 are still depended upon system libraries.

This was disabled in microsoft#14275 because it caused the relevant libraries (ibus, glib, ...) to be depended on transitively.

Nowadays, SDL2 loads these completely at runtime, only needing the headers and such at compile time, so it's safe to re-enable this again.

Ibus is gated behind the "ibus" feature, which like the existing x11/wayland ones warn at installation that you need the necessary system packages to use it. The ibus feature is enabled by default.
@FrankXie05 FrankXie05 self-assigned this Feb 13, 2023
@FrankXie05 FrankXie05 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Feb 13, 2023
message(WARNING "You will need to install Wayland dependencies to use feature wayland:\nsudo apt install libwayland-dev libxkbcommon-dev libegl1-mesa-dev\n")
endif()
if ("ibus" IN_LIST FEATURES)
message(WARNING "You will need to install ibus dependencies to use feature ibus:\nsudo apt install libibus-1.0-dev\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

We can currently accept adding a warning message to require users to install system packages to use this feature. I will test this feature like dg0yt said.

@FrankXie05 FrankXie05 added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Feb 13, 2023
@JavierMatosD JavierMatosD merged commit bf67efb into microsoft:master Feb 13, 2023
@PJB3005 PJB3005 deleted the sdl2-ibus branch February 13, 2023 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants