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

[vcpkg] Add vcpkg_install_copyright() portfile function #25239

Merged
merged 29 commits into from
Jul 13, 2022
Merged

[vcpkg] Add vcpkg_install_copyright() portfile function #25239

merged 29 commits into from
Jul 13, 2022

Conversation

Thomas1664
Copy link
Contributor

@Thomas1664 Thomas1664 commented Jun 14, 2022 •
edited
Loading

Currently, vcpkg allows ports to create a copyright directory. This is a bug in vcpkg is and addressed in this PR: microsoft/vcpkg-tool#584
Rather than having a copyright directory, we want to merge all license files into a single copyright file. The cmake function vcpkg_concat_copyright simplifies this.
Unfortunately, some ports in this catalogue have a copyright directory. Thus, the vcpkg-tool PR linked above will break CI. To prevent this, we need to do a world rebuild in this PR from microsoft/vcpkg-tool#584 .

  • Add a cmake function to merge copyright files if there are multiple of them: vcpkg_concat_copyright
  • Document vcpkg_concat_copyright
  • Document how to handle multiple copyright files in the maintainers guide
  • Fix ports (other PRs)

@Thomas1664
Copy link
Contributor Author

@BillyONeal May I get a full world rebuild based on microsoft/vcpkg-tool#584 to test which ports are affected?

@Thomas1664 Thomas1664 changed the title [vcpkg tool] Only allow copyright files [world rebuild][vcpkg tool] Only allow copyright files Jun 14, 2022
@Thomas1664 Thomas1664 mentioned this pull request Jun 14, 2022
Merged
@JonLiu1993 JonLiu1993 self-assigned this Jun 15, 2022
@JonLiu1993 JonLiu1993 added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Jun 15, 2022
@JonLiu1993
Copy link
Member

JonLiu1993 commented Jun 22, 2022 •
edited
Loading

@Thomas1664 , If this pr is ready to review, please let me know, thanks

@Thomas1664
Copy link
Contributor Author

@Thomas1664 , If these pr is ready to review, please let me know, thanks

@JonLiu1993 ready for review

@Thomas1664 @BillyONeal
Tell users to use the correct way
752c91b 
Co-authored-by: Billy O'Neal <bion@microsoft.com>
BillyONeal
BillyONeal previously approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I really like this, thank you!

ras0219-msft
ras0219-msft previously requested changes Jun 22, 2022
View reviewed changes
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

docs/maintainers/vcpkg_install_copyright.md Outdated Show resolved Hide resolved
docs/maintainers/vcpkg_install_copyright.md Show resolved Hide resolved
docs/maintainers/vcpkg_install_copyright.md Outdated Show resolved Hide resolved
docs/maintainers/vcpkg_install_copyright.md Outdated Show resolved Hide resolved
docs/maintainers/vcpkg_install_copyright.md Outdated Show resolved Hide resolved
@Thomas1664 Thomas1664 mentioned this pull request Jun 22, 2022
Merged
@dg0yt
Copy link
Contributor

dg0yt commented Jun 23, 2022

I wonder if I would ever use vcpkg_install_copyright if there is no way to give additional text. See libiconv for a potential use case. (And see pdal for a complex case which is probably out of scope.)

github-actions[bot]
github-actions bot previously approved these changes Jun 23, 2022
View reviewed changes
@Thomas1664
Copy link
Contributor Author

Thomas1664 commented Jun 23, 2022 •
edited
Loading

I wonder if I would ever use vcpkg_install_copyright if there is no way to give additional text. See libiconv for a potential use case. (And see pdal for a complex case which is probably out of scope.)

I added the parameter COMMENT that will do exactly want you want. For pdal you could write something like a/path: MIT

BillyONeal
BillyONeal previously approved these changes Jul 6, 2022
View reviewed changes
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

This approval is conditional upon the suggestions being applied. Thanks again!

scripts/cmake/vcpkg_install_copyright.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_install_copyright.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_install_copyright.cmake Outdated Show resolved Hide resolved
scripts/cmake/vcpkg_install_copyright.cmake Outdated Show resolved Hide resolved
@BillyONeal BillyONeal dismissed ras0219-msft’s stale review July 6, 2022 19:00

The content has changed and Robert is out with COIVD

@Thomas1664 Thomas1664 dismissed stale reviews from BillyONeal and github-actions via fc02ce2 July 6, 2022 19:37
BillyONeal
BillyONeal previously approved these changes Jul 6, 2022
View reviewed changes
JackBoosY
JackBoosY previously approved these changes Jul 7, 2022
View reviewed changes
@Thomas1664 Thomas1664 dismissed stale reviews from JackBoosY and BillyONeal via 925ffce July 7, 2022 04:38
github-actions[bot]
github-actions bot previously approved these changes Jul 7, 2022
View reviewed changes
@Thomas1664
Copy link
Contributor Author

I will merge master again after #25735 is merged.

@Thomas1664
Copy link
Contributor Author

Thanks for merging #25735!

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jul 13, 2022
@vicroms vicroms merged commit c69f2b9 into microsoft:master Jul 13, 2022
@Thomas1664 Thomas1664 mentioned this pull request Jul 13, 2022
Merged
@Thomas1664 Thomas1664 deleted the chartdir-ci branch July 14, 2022 05:39
p12tic pushed a commit to p12tic/vcpkg that referenced this pull request Sep 14, 2022
@Thomas1664 @BillyONeal @p12tic
[vcpkg] Add vcpkg_install_copyright() portfile function (microsoft#…
b465ac7 
…25239)

* [vcpkg tool] Add vcpkg_install_copyright

* Make sure FILE_LIST is provided

* relative to ${SOURCE_PATH}

* Add documentation

* Add to table of contents

* Relative paths was a bad idea.

* Tell users to use the correct way

Co-authored-by: Billy O'Neal <bion@microsoft.com>

* Fix docs

* Add parameter COMMENT

* Rename to vcpkg_concat_copyright

* Fix escape

* Revert "Fix escape"

This reverts commit 53f1636.

* Revert "Rename to vcpkg_concat_copyright"

This reverts commit 6ce9152.

* Fix escape

* Add support for single copyright file

* Update docs

* Make comment less confusing

* [ci skip] Billy CR

* [ci skip] Format

* Remove explicit checks for STREQUAL ""

* Add error msg if file doesn't exist

Co-authored-by: Billy O'Neal <bion@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cenit cenit cenit left review comments

@dg0yt dg0yt dg0yt left review comments

@vicroms vicroms vicroms approved these changes

@github-actions github-actions[bot] github-actions approved these changes

@JackBoosY JackBoosY JackBoosY approved these changes

@JonLiu1993 JonLiu1993 JonLiu1993 approved these changes

@BillyONeal BillyONeal BillyONeal left review comments

@ras0219-msft ras0219-msft Awaiting requested review from ras0219-msft

Assignees

@JackBoosY JackBoosY

@JonLiu1993 JonLiu1993

Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed 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
8 participants
@Thomas1664 @JonLiu1993 @dg0yt @BillyONeal @JackBoosY @vicroms @cenit @ras0219-msft