Skip to content

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

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

Merged
BillyONeal merged 8 commits into from
Jul 20, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e00a129
Make sure file is a file / directory
Thomas1664 Jun 11, 2022
ff259a4
Don't error on copyright is dir
Thomas1664 Jun 12, 2022
862e286
Apply diff
Thomas1664 Jun 12, 2022
6fe49f2
Avoid multiple fs calls
Thomas1664 Jun 14, 2022
d347e52
Format diff
Thomas1664 Jun 14, 2022
e9bf542
Merge remote-tracking branch 'origin/main' into fix-fs-exists
Thomas1664 Jul 14, 2022
fa741f8
Merge localized msg
Thomas1664 Jul 14, 2022
f0b8111
Merge remote-tracking branch 'origin/main' into fix-fs-exists
BillyONeal Jul 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/vcpkg/base/messages.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,7 @@ namespace vcpkg
"{command_line}\n"
"failed with the following results:");
DECLARE_MESSAGE(CompressFolderFailed, (msg::path), "", "Failed to compress folder \"{path}\":");
DECLARE_MESSAGE(CopyrightIsDir, (msg::path), "", "`{path}` being a directory is deprecated.");
DECLARE_MESSAGE(CouldNotDeduceNugetIdAndVersion,
(msg::path),
"",
Expand Down
1 change: 1 addition & 0 deletions locales/messages.en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
"CmakeTargetsExcluded": "note: {count} additional targets are not displayed.",
"CommandFailed": "command:\n{command_line}\nfailed with the following results:",
"CompressFolderFailed": "Failed to compress folder \"{path}\":",
"CopyrightIsDir": "`{path}` being a directory is deprecated.",
"CouldNotDeduceNugetIdAndVersion": "Could not deduce nuget id and version from filename: {path}",
"CreatedNuGetPackage": "Created nupkg: \"{path}\"",
"CurlReportedUnexpectedResults": "curl has reported unexpected results to vcpkg and vcpkg cannot continue.\nPlease review the following text for sensitive information and open an issue on the Microsoft/vcpkg GitHub to help fix this problem!\ncmd: {command_line}\n=== curl output ===\n{actual}\n=== end curl output ===",
Expand Down
2 changes: 2 additions & 0 deletions locales/messages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@
"_CommandFailed.comment": "An example of {command_line} is vcpkg install zlib.",
"CompressFolderFailed": "Failed to compress folder \"{path}\":",
"_CompressFolderFailed.comment": "An example of {path} is /foo/bar.",
"CopyrightIsDir": "`{path}` being a directory is deprecated.",
"_CopyrightIsDir.comment": "An example of {path} is /foo/bar.",
"CouldNotDeduceNugetIdAndVersion": "Could not deduce nuget id and version from filename: {path}",
"_CouldNotDeduceNugetIdAndVersion.comment": "An example of {path} is /foo/bar.",
"CreatedNuGetPackage": "Created nupkg: \"{path}\"",
Expand Down
1 change: 1 addition & 0 deletions src/vcpkg/base/messages.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,7 @@ namespace vcpkg
REGISTER_MESSAGE(CMakeTargetsUsageHeuristicMessage);
REGISTER_MESSAGE(CMakeToolChainFile);
REGISTER_MESSAGE(CommandFailed);
REGISTER_MESSAGE(CopyrightIsDir);
REGISTER_MESSAGE(CompressFolderFailed);
REGISTER_MESSAGE(CouldNotDeduceNugetIdAndVersion);
REGISTER_MESSAGE(CreatedNuGetPackage);
Expand Down
2 changes: 1 addition & 1 deletion src/vcpkg/install.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,7 @@ namespace vcpkg
std::error_code ec;

auto usage_file = installed.usage_file(bpgh.spec);
if (fs.exists(usage_file, IgnoreErrors{}))
if (fs.is_regular_file(usage_file))
Copy link
Contributor

@autoantwort autoantwort Jul 20, 2022

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

@Thomas1664 Thomas1664 Jul 20, 2022

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

@BillyONeal BillyONeal Jul 20, 2022

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

@BillyONeal BillyONeal Jul 20, 2022

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

@autoantwort autoantwort Jul 20, 2022

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

@Thomas1664 Thomas1664 Jul 20, 2022

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

@autoantwort autoantwort Jul 20, 2022

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)

{
ret.usage_file = true;
auto contents = fs.read_contents(usage_file, ec);
Expand Down
8 changes: 6 additions & 2 deletions src/vcpkg/postbuildlint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,10 +342,14 @@ namespace vcpkg::PostBuildLint
{
const auto packages_dir = paths.packages() / spec.dir();
const auto copyright_file = packages_dir / "share" / spec.name() / "copyright";
if (fs.exists(copyright_file, IgnoreErrors{}))

switch (fs.status(copyright_file, IgnoreErrors{}))
{
return LintStatus::SUCCESS;
case FileType::regular: return LintStatus::SUCCESS; break;
case FileType::directory: msg::println_warning(msgCopyrightIsDir, msg::path = "copyright"); break;
default: break;
}

const auto current_buildtrees_dir = paths.build_dir(spec);
const auto current_buildtrees_dir_src = current_buildtrees_dir / "src";

Expand Down