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

[libusb] Add disable-udev feature #35328

Merged
merged 9 commits into from
Nov 29, 2023
Merged

[libusb] Add disable-udev feature #35328

merged 9 commits into from
Nov 29, 2023

Conversation

simolis3
Copy link
Contributor

@simolis3 simolis3 commented Nov 26, 2023 •
edited
Loading

  • 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.

Adds new feature for libusb port to allow building with --disable-udev flag. Default behavior still remains enabling udev (explicit --enable-udev flag is added in this case for clarity)

@simolis3
Copy link
Contributor Author

@microsoft-github-policy-service agree

JoergAtGithub
JoergAtGithub reviewed Nov 26, 2023
View reviewed changes
ports/libusb/vcpkg.json Outdated
Comment on lines 16 to 19
"features": {
"disable-udev":{
"description": "Disable udev"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be defined as platform specific feature, as udev is only available on Unix?

Copy link
Contributor

Choose a reason for hiding this comment

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

disable-udev is not acceptable: "Features must be treated as additive functionality."

There can be an udev feature, it can define what it supports, and it can be in default-features subject to platform expressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great comments! I'll have these fixed and manually tested in the upcoming days. It's my first time contributing :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed disable-udev to udev feature and set this as a default feature for linux platform (and it supports only linux platform). I'm using this on linux, but was wondering if the platform expression should still be something like !windows to support other non windows platforms, any opinions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm using this on linux, but was wondering if the platform expression should still be something like !windows to support other non windows platforms, any opinions?

It was limited to linux before, so you don't add a new limitation.
And actually it is easier now to test new platforms: You can ignore supports at the command line, with --allow-unsupported.

dg0yt
dg0yt reviewed Nov 26, 2023
View reviewed changes
ports/libusb/portfile.cmake Outdated
Comment on lines 14 to 18
vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
FEATURES
"disable-udev" DISABLE_UDEV
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
FEATURES
"disable-udev" DISABLE_UDEV
)

vcpkg_check_features is for cmake options (via OUT_FEATURE_OPTIONS).
There is a side effect on DISABLE_UDEV etc. but IMO it shouldn't be used just for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in latest commits

ports/libusb/portfile.cmake Outdated
@@ -41,9 +46,15 @@ if(VCPKG_TARGET_IS_WINDOWS AND NOT VCPKG_TARGET_IS_MINGW)
vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/debug/lib/pkgconfig/libusb-1.0.pc" " -lusb-1.0" " -llibusb-1.0")
endif()
else()
if(${DISABLE_UDEV})
Copy link
Contributor

Choose a reason for hiding this comment

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

The usual pattern is:

Suggested change
if(${DISABLE_UDEV})
if("feature-name" IN_LIST FEATURES)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in latest commits

ports/libusb/vcpkg-cmake-wrapper.cmake Outdated
@@ -8,7 +8,7 @@ select_library_configurations(LIBUSB)

set(LIBUSB_INCLUDE_DIRS "${LIBUSB_INCLUDE_DIR}")

if (@VCPKG_TARGET_IS_LINUX@)
if (@VCPKG_TARGET_IS_LINUX@ AND NOT ${DISABLE_UDEV})
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that we use ${var} for late-binding evaluation, and @var@ for early substitutions (configure_file(...@ONLY)).

But maybe we restructure this section: Construct LIBUSB_LINK_LIBRARIES in the portfile (subject to features), and replace the whole if block with

list(APPEND LIBUSB_LIBRARIES @LIBUSB_LINK_LIBRARIES@)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Section restructured and LIBUSB_LINK_LIBRARIES constructed in portfile in latest commits.

@Cheney-W Cheney-W added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Nov 27, 2023
@Cheney-W
Copy link
Contributor

Please run:

vcpkg x-add-version libusb --overwrite-version
git add . 
git commit -m "Update version database"

to add the new version.

@Cheney-W Cheney-W marked this pull request as draft November 27, 2023 09:18
dg0yt
dg0yt reviewed Nov 27, 2023
View reviewed changes
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 someone else must approve.

One more suggestion: append to MAKE_OPTIONS, and formally initialize the lists.

ports/libusb/portfile.cmake Outdated
Comment on lines 44 to 49
if("udev" IN_LIST FEATURES)
set(MAKE_OPTIONS "--enable-udev")
list(APPEND LIBUSB_LINK_LIBRARIES udev)
else()
set(MAKE_OPTIONS "--disable-udev")
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if("udev" IN_LIST FEATURES)
set(MAKE_OPTIONS "--enable-udev")
list(APPEND LIBUSB_LINK_LIBRARIES udev)
else()
set(MAKE_OPTIONS "--disable-udev")
endif()
vcpkg_list(SET MAKE_OPTIONS)
vcpkg_list(SET LIBUSB_LINK_LIBRARIES)
if("udev" IN_LIST FEATURES)
vcpkg_list(APPEND MAKE_OPTIONS "--enable-udev")
vcpkg_list(APPEND LIBUSB_LINK_LIBRARIES udev)
else()
vcpkg_list(APPEND MAKE_OPTIONS "--disable-udev")
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented in latest commit

@simolis3
Copy link
Contributor Author

Noticed that this might help with the problem described here: #34343

@simolis3 simolis3 marked this pull request as ready for review November 27, 2023 13:38
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Nov 28, 2023
@vicroms vicroms merged commit cabba0d into microsoft:master Nov 29, 2023
15 checks passed
@JonLiu1993 JonLiu1993 mentioned this pull request Nov 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dg0yt dg0yt dg0yt left review comments

@JoergAtGithub JoergAtGithub JoergAtGithub left review comments

@Cheney-W Cheney-W Cheney-W approved these changes

Assignees

@Cheney-W Cheney-W

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@simolis3 @Cheney-W @dg0yt @JoergAtGithub @vicroms