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

[sdl2] Make dbus dependency optional. #40633

Merged
merged 2 commits into from
Aug 29, 2024
Merged

Conversation

dsvensson
Copy link
Contributor

@dsvensson dsvensson commented Aug 25, 2024

Instead of having dbus as a mandatory dependency, make it optional, but enabled by the default features to keep behavior for people who just depend on sdl2 without disabling default features. Trims about 85% off the build time if disabled.

Fixes #40632

@dsvensson dsvensson force-pushed the patch-1 branch 3 times, most recently from dc5a623 to 002e211 Compare August 25, 2024 19:17
@dsvensson
Copy link
Contributor Author

@microsoft-github-policy-service agree

@dsvensson
Copy link
Contributor Author

@FrankXie05 Not sure what's up with the failed Windows builds given that they have nothing to do with dbus.

@FrankXie05 FrankXie05 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Aug 26, 2024
@FrankXie05
Copy link
Contributor

@dsvensson dsvensson force-pushed the patch-1 branch 2 times, most recently from cb32d09 to aadcf7a Compare August 26, 2024 10:12
@dsvensson
Copy link
Contributor Author

@FrankXie05 That did the trick, thanks!

@dg0yt
Copy link
Contributor

dg0yt commented Aug 26, 2024

The choice of the dbus feature must also be handled in the portfile.cmake, or you end up with installation order and system libs surprises.

@dsvensson
Copy link
Contributor Author

@dg0yt Could you explain how, I'm not sure I follow.

@dg0yt
Copy link
Contributor

dg0yt commented Aug 26, 2024

ATM this PR just make it optional to have the dbus port installed by vcpkg before the sdl2 port.
But what is needed is to ensure that sdl2 never uses dbus unless the feature is selected (explicitly or implicitly). When I run:

vcpkg install dbus
vcpkg install sdl2[core]

sdl2 must not use dbus. (Similar for system-provided dbus.)

If there is a CMake option to control this goal, you can use that option. If there is find_package, you could use CMAKE_DISABLE_FIND_PACKAGE_<pkg>. Worst case, you must patch.

@dsvensson
Copy link
Contributor Author

Doesn't the same apply to x11, Wayland etc. If you happen to not have the system headers installed they are silently disabled even if requested?

@dsvensson
Copy link
Contributor Author

dsvensson commented Aug 26, 2024

@dg0yt https://github.com/libsdl-org/SDL/blob/SDL2/CMakeLists.txt#L1593-L1601

root@765c033c3c6c:/vcpkg/buildtrees/sdl2# grep -i dbus *
config-x64-linux-dbg-CMakeCache.txt.log:SDL_DBUS:BOOL=OFF

using:

FROM debian:unstable

RUN apt-get update && apt-get -qq install --no-install-recommends build-essential git cmake ninja-build autoconf libtool python3 python3-jinja2 curl zip unzip tar ca-certificates libltdl-dev pkg-config

RUN <<EOF cat >> vcpkg.json
{
  "\$schema": "https://raw.githubusercontent.com/microsoft/vcpkg-tool/main/docs/vcpkg.schema.json",
  "name": "ezquake",
  "version": "1.0.0",
  "dependencies": [
    { "name": "dbus", "default-features": false },
    { "name": "sdl2", "default-features": false }
  ]
}
EOF

#RUN git clone https://github.com/microsoft/vcpkg
RUN git clone https://github.com/dsvensson/vcpkg && (cd vcpkg; git reset --hard origin/patch-1)
RUN VCPKG_DISABLE_METRICS=1 vcpkg/bootstrap-vcpkg.sh && vcpkg/vcpkg install

....and removing the dbus line and passing "features": ["dbus"] to sdl2 corectly enables it.

@dsvensson
Copy link
Contributor Author

@FrankXie05 @dg0yt Anything else that stands out?

ports/sdl2/portfile.cmake Outdated Show resolved Hide resolved
ports/sdl2/portfile.cmake Outdated Show resolved Hide resolved
@dsvensson
Copy link
Contributor Author

@dg0yt Could this be the final version? Or there's more CI torture left 😂

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

LGTM but somebody else must approve.
Thanks @dsvensson.

@dsvensson
Copy link
Contributor Author

@FrankXie05 what do you think, ready?

@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Aug 28, 2024
@FrankXie05
Copy link
Contributor

@dsvensson Thanks for this PR, and @dg0yt thanks for review. :)

@dsvensson
Copy link
Contributor Author

@FrankXie05 Now that it's marked as reviewed, approved etc, what phases are left until it's merged? Just trying to get an understanding of the PR workflow in this project as I have some other ports that I carry some changes for so just want to align my expectations.

@vicroms vicroms merged commit de99811 into microsoft:master Aug 29, 2024
16 checks passed
@vicroms
Copy link
Member

vicroms commented Aug 29, 2024

@dsvensson after PR has been marked info:reviewed a member of the Microsoft/vcpkg team gives a final review and clicks the merge button.

@anders-wind anders-wind mentioned this pull request Sep 2, 2024
7 tasks
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:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[sdl2] build failure on debian sid, make dbus optional dep of sdl2
4 participants