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

[lunasvg] Add usage #30196

Merged
merged 24 commits into from
Jul 14, 2023
Merged

[lunasvg] Add usage #30196

merged 24 commits into from
Jul 14, 2023

Conversation

JonLiu1993
Copy link
Member

@JonLiu1993 JonLiu1993 commented Mar 15, 2023

Fixes #30186

Lunasvg does not export anything after installation, I add the usage.
Tested the usage successfully by lunasvg:x64-windows:

lunasvg provides CMake targets:

    # this is heuristically generated, and may not be correct
    find_package(unofficial-lunasvg CONFIG REQUIRED)
    target_link_libraries(main PRIVATE unofficial::lunasvg::lunasvg)
  • 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.

@JonLiu1993 JonLiu1993 added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:internal This PR or Issue was filed by the vcpkg team. labels Mar 15, 2023
@JonLiu1993 JonLiu1993 marked this pull request as ready for review March 15, 2023 09:42
@Cheney-W Cheney-W marked this pull request as draft March 15, 2023 09:50
@JonLiu1993 JonLiu1993 marked this pull request as ready for review March 16, 2023 06:06
@JonLiu1993 JonLiu1993 requested a review from Cheney-W March 16, 2023 06:06
Cheney-W
Cheney-W previously approved these changes Mar 16, 2023
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Mar 16, 2023
@dg0yt
Copy link
Contributor

dg0yt commented Mar 16, 2023

Does this config help with any usage requirements beyond what documenting find_path and find_library in usage could do?
Adding unofficial config everywhere (and not upstreaming it) effectively still creates vcpkg lock-in.

@JonLiu1993
Copy link
Member Author

Does this config help with any usage requirements beyond what documenting find_path and find_library in usage could do? Adding unofficial config everywhere (and not upstreaming it) effectively still creates vcpkg lock-in.

@dg0yt, Thanks for your review, what exactly are you worthy of meeting any other requirements? I just checked locally whether the files generated under its packages\ folder are correct, and tested its exported usage locally.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 16, 2023

I didn't claim it wouldn't be working.
This change is adding a patch, which is intrusive, to add an unofficial CMake config which doesn't exist outside the vcpgk world. I'm not convinced that this is more beneficial than useful.

With regard to transitive usage requirements, I found only the following: C++ linkage, libm for non-windows.

With regard to upstream, sammycage/lunasvg#84 may need support.

@Pospelove
Copy link
Contributor

Thanks for taking a look at this port

Please consider also updating to the latest commit. The current version is outdated and lacks critical patches Like UB fix sammycage/lunasvg#123 and much more

Thanks again

@JonLiu1993 JonLiu1993 removed the info:reviewed Pull Request changes follow basic guidelines label Mar 17, 2023
@JonLiu1993 JonLiu1993 marked this pull request as draft March 17, 2023 09:57
@aeris170
Copy link

Sorry to rush things but, could we merge this?

@JonLiu1993 JonLiu1993 changed the title [lunasvg] update to latest commit to add usage [lunasvg] update to version 2.3.8 and add usage May 30, 2023
@JonLiu1993 JonLiu1993 marked this pull request as ready for review May 31, 2023 01:45
@JonLiu1993 JonLiu1993 requested a review from Cheney-W May 31, 2023 01:45
Cheney-W
Cheney-W previously approved these changes May 31, 2023
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label May 31, 2023
@JonLiu1993
Copy link
Member Author

Since the addition of upstream usage caused issue for users, the latest upstream version restored the changes about usage in CMakeLists. I updated to the latest version and added a patch to provide unofficial usage.

@dg0yt
Copy link
Contributor

dg0yt commented May 31, 2023

The upstream issues were caused by build-internal changes, not by the exported config.
I would prefer you wouldn't add an unofficial config patch at this point and submitted a PR upstream instead.
I doesn't hurt to merge one more port update without exported config.

@Cheney-W Cheney-W removed the info:reviewed Pull Request changes follow basic guidelines label Jun 1, 2023
@Cheney-W Cheney-W marked this pull request as draft June 1, 2023 01:49
@JonLiu1993
Copy link
Member Author

I have commit a PR for Upstream sammycage/lunasvg#132

@JonLiu1993
Copy link
Member Author

JonLiu1993 commented Jul 13, 2023

I submitted a PR upstream, but there has been no response for a month and a half, I think we can merge this PR first for users to use.

@JonLiu1993 JonLiu1993 requested a review from Cheney-W July 13, 2023 07:26
@JonLiu1993 JonLiu1993 changed the title [lunasvg] update to version 2.3.8 and add usage [lunasvg] Add usage Jul 13, 2023
@JonLiu1993 JonLiu1993 marked this pull request as ready for review July 13, 2023 07:27
Comment on lines +12 to +16
},
{
"git-tree": "bf20380ca537151f7d1e02a6ca5b19c302db18c7",
"version": "2.3.1",
"port-version": 0
Copy link
Member Author

Choose a reason for hiding this comment

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

PR #32510 Deleted the historical "git-tree" when updating lunasvg, I restore it here.

Copy link
Member

Choose a reason for hiding this comment

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

Oops 😳. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

My mistake, thanks for the fix!

@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Jul 14, 2023
@BillyONeal BillyONeal merged commit bffafa8 into microsoft:master Jul 14, 2023
@BillyONeal
Copy link
Member

Thanks!

@aeris170
Copy link

Thanks for merging fast!

@JonLiu1993 JonLiu1993 deleted the dev/Jon/lunasvg branch July 17, 2023 01:38
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:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[lunasvg] Port does not expose anything when installed
8 participants