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-mixer] Fix usage #26083

Merged
merged 6 commits into from
Aug 4, 2022
Merged

[sdl2-mixer] Fix usage #26083

merged 6 commits into from
Aug 4, 2022

Conversation

Thomas1664
Copy link
Contributor

@Thomas1664 Thomas1664 commented Jul 31, 2022

Describe the pull request

github-actions[bot]
github-actions bot previously approved these changes Jul 31, 2022
@Cheney-W Cheney-W self-assigned this Aug 1, 2022
@Cheney-W Cheney-W added category:port-bug The issue is with a library, which is something the port should already support and removed category:port-bug The issue is with a library, which is something the port should already support labels Aug 1, 2022
@Cheney-W
Copy link
Contributor

Cheney-W commented Aug 1, 2022

I think this PR is invalid, this issue #26064 is not a vcpkg issue. I have tested sdl2-mixer in my side and the original usage works fine.

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.

The PR is valid even if it doesn't fix #26064. Neither users nor heuristics can figure out

target_link_libraries($<IF:$<TARGET_EXISTS:SDL2_mixer::SDL2_mixer>,SDL2_mixer::SDL2_mixer,SDL2_mixer::SDL2_mixer-static>)

so it takes an explicit usage file.

@@ -0,0 +1,3 @@
sdl2-mixer provides CMake targets:
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, I would suggest to add a new line after this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We should fix the auto-generated usage as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see, the # this is heuristically ... was added only a short time ago.

Copy link
Contributor Author

@Thomas1664 Thomas1664 Aug 1, 2022

Choose a reason for hiding this comment

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

@dg0yt I created microsoft/vcpkg-tool#653 . The usage generation looks weird: We search in every CMake file in share for add_library and add the name(s) of the library to the list of exported targets. However, CMake just searches for <namespace>/target-name-config.cmake and <namespace>/target-nameConfig.cmake. I think we should adopt this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Thomas1664 See #20190 for test port candidates and proposals for improvement (#20190 (comment)).

@Thomas1664
Copy link
Contributor Author

Just for reference the suggested output is

Total elapsed time: 15.44 s

sdl2-mixer provides CMake targets:
    # this is heuristically generated, and may not be correct
    find_package(SDL2_mixer CONFIG REQUIRED)
    # note: 6 additional targets are not displayed.
    target_link_libraries(main PRIVATE FLAC::FLAC MPG123::mpg123 libxmp::libxmp tremor::tremor)

This is wrong because there aren't any sdl2-mixer libraries.

github-actions[bot]
github-actions bot previously approved these changes Aug 1, 2022
@Cheney-W Cheney-W added 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 and removed invalid labels Aug 2, 2022
@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Aug 4, 2022
@BillyONeal BillyONeal merged commit 59b861a into microsoft:master Aug 4, 2022
@Thomas1664 Thomas1664 deleted the sdl2-mixer branch August 4, 2022 18:28
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Aug 5, 2022
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.

4 participants