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

Change the copyright post-build-lint to require a regular file. #584

Merged
merged 8 commits into from
Jul 20, 2022

Conversation

Thomas1664
Copy link
Contributor

@Thomas1664 Thomas1664 commented Jun 11, 2022

Changes:

  • Only try to read usage if it is a regular file
  • Post build lint: Print a warning if copyright is a directory

Note:
I checked the file lists for copyright/. There are some ports that have their "copyright" inside copyright/ and only provide one license file. Therefore, this change breaks some ports. I suggest to create a PR where I fix those ports based on a version of vcpkg based on this PR before this PR gets merged.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 11, 2022

I checked the file lists for copyright/. There are some ports that have their "copyright" inside copyright/ and only provide one license file. Therefore, this change breaks some ports. I suggest to create a PR where I fix those ports based on a version of vcpkg based on this PR before this PR gets merged.

  • copyright being a directory is deprecated. Everything shall be in a single file.
  • Some ports may install additional license files depending on features. A frequent pattern: libraries (core) are LGPL, programs (tools) are GPL.

Cf. https://github.com/microsoft/vcpkg/blob/a7f7a01c6aa274996d44b362d9a289e5bba698f4/ports/libiconv/portfile.cmake#L53-L68

@Thomas1664
Copy link
Contributor Author

copyright being a directory is deprecated. Everything shall be in a single file.
I can't find any documentation about this. @BillyONeal Is that true?

  • Some ports may install additional license files depending on features. A frequent pattern: libraries (core) are LGPL, programs (tools) are GPL.

Mostly kf5* ports are affected by this. They are responsible for ~240 hits (on x64-windows).
Only 6 other ports are affected as well.

We should add a cmake function that merges all copyright files into a single file vcpkg_merge_and_install_copyright(). Or we make a function vcpkg_install_copyright() that every port has to use and is capable of merging license files as well.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 12, 2022

microsoft/vcpkg#19227 (comment).
Maybe (re-start) with documentation first.

@Thomas1664 Thomas1664 marked this pull request as ready for review June 12, 2022 12:17
@Thomas1664
Copy link
Contributor Author

Now that the vcpkg tool knows about licenses and creates vcpkg.spdx.json we may want to think again how to handle license files

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved
src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved
@BillyONeal BillyONeal changed the title Make sure file is a file / directory Change the copyright post-build-lint to require a regular file. Jun 24, 2022
@BillyONeal BillyONeal added the depends:different-pr This PR depends on a different PR which has been filed label Jun 24, 2022
@BillyONeal
Copy link
Member

Thank you <3

@Thomas1664
Copy link
Contributor Author

Depends on microsoft/vcpkg#25759

@Thomas1664
Copy link
Contributor Author

Thomas1664 commented Jul 14, 2022

@BillyONeal I checked the file lists and didn't find any copyright directory. I think this PR can be merged now?

@BillyONeal
Copy link
Member

@BillyONeal I checked the file lists and didn't find any copyright directory. I think this PR can be merged now?

Check queued: https://dev.azure.com/vcpkg/public/_build/results?buildId=75362&view=results

@Thomas1664
Copy link
Contributor Author

Thomas1664 commented Jul 19, 2022

Check queued: https://dev.azure.com/vcpkg/public/_build/results?buildId=75362&view=results

Not found any copyright/ in

@BillyONeal BillyONeal merged commit a8de335 into microsoft:main Jul 20, 2022
@BillyONeal
Copy link
Member

Thanks for your contributions!

@Thomas1664 Thomas1664 deleted the fix-fs-exists branch July 20, 2022 20:38
if (fs.exists(usage_file, IgnoreErrors{}))
if (fs.is_regular_file(usage_file))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm it is a supported use case to use a new vcpkg-tool with an old port via versioning?

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, you will just get a warning if you try to install an old port that was affected by this. Just vcpkg CI will treat it as an error.
Furthermore, I think we shouldn't ever try to support ports that make use of bugs in our tool.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's why post-build checks no longer fail the run

Copy link
Member

Choose a reason for hiding this comment

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

Furthermore, I think we shouldn't ever try to support ports that make use of bugs in our tool.

That is not our policy. No changes must mean no changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

My only concern was that the package has no license file after this change (and that the resulting package changes but the binary hash is the same, but I don't care).

Furthermore, I think we shouldn't ever try to support ports that make use of bugs in our tool.

Totally agree. I just wanted to ask :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only concern was that the package has no license file after this change (and that the resulting package changes but the binary hash is the same, but I don't care).

That is not possible. The copyright file gets installed by cmake. If cmake installed it, it won't be removed. During post build checks we only check that everything is OK but don't clean anything up if something is broken,

Copy link
Contributor

Choose a reason for hiding this comment

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

The copyright file gets installed by cmake. If cmake installed it, it won't be removed.

Sorry my mistake. I only read install.cpp and copyright and thought this code installed somehow the license files (actually I know that they are installed by cmake)

@BillyONeal
Copy link
Member

We're doing a release to pick this up: https://github.com/microsoft/vcpkg-tool/releases/tag/2022-07-21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depends:different-pr This PR depends on a different PR which has been filed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants