-
Notifications
You must be signed in to change notification settings - Fork 292
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
[post build lint] Check for absolute paths #172
Changes from 38 commits
863231d
15778b6
9212d1b
326672e
cef8cb0
761dc17
7fd595a
7e9fdd0
f58d9ae
6840f39
d7f0f47
42c62d7
80704ee
2c1fb86
cd19401
1fd5a30
6154d36
47787c5
86072b7
ea1e189
10049d7
03d24cd
b51e0b1
5d19692
b48fd0f
81c991d
aa5264b
8b2cc5c
d58b64e
6db65d5
04e77f5
dce3e55
9df3a83
dec596d
20a0747
cc4cbc8
7b535b4
dfa58d3
3fd3ea2
024b814
a1b3aa1
6421db4
bdcb84c
2da94b4
535e47a
72476ba
646525c
9b0c5f8
2329d8d
771a5a1
1994010
2df175b
23c9e7d
0a5c013
028dd92
a2a42d5
4092884
22c3d0e
c1188c6
f8b77a2
0937195
bc23f1f
298a2f4
4fabb06
ed7e9c8
cc3f741
ccf6b23
5222349
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,10 +1,12 @@ | ||||||
#include <vcpkg/base/cofffilereader.h> | ||||||
#include <vcpkg/base/files.h> | ||||||
#include <vcpkg/base/messages.h> | ||||||
#include <vcpkg/base/system.print.h> | ||||||
#include <vcpkg/base/system.process.h> | ||||||
#include <vcpkg/base/util.h> | ||||||
|
||||||
#include <vcpkg/build.h> | ||||||
#include <vcpkg/installedpaths.h> | ||||||
#include <vcpkg/packagespec.h> | ||||||
#include <vcpkg/postbuildlint.buildtype.h> | ||||||
#include <vcpkg/postbuildlint.h> | ||||||
|
@@ -1025,6 +1027,101 @@ namespace vcpkg::PostBuildLint | |||||
return LintStatus::SUCCESS; | ||||||
} | ||||||
|
||||||
static LintStatus check_no_absolute_paths_in(const Filesystem& fs, const Path& dir, Span<Path> absolute_paths) | ||||||
{ | ||||||
static constexpr StringLiteral extensions[] = {"h", "hpp", "hxx", "py", "sh", "cmake", "pc", "cfg", "conf"}; | ||||||
std::vector<std::pair<Path, std::string>> files_and_contents; | ||||||
autoantwort marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
for (auto& path : fs.get_regular_files_recursive(dir, IgnoreErrors{})) | ||||||
{ | ||||||
if (path.extension().empty()) | ||||||
{ | ||||||
auto contents = fs.read_contents(path, VCPKG_LINE_INFO); | ||||||
if (Strings::starts_with(contents, "#!")) | ||||||
{ | ||||||
files_and_contents.emplace_back(std::move(path), std::move(contents)); | ||||||
} | ||||||
} | ||||||
else if (Util::contains(extensions, path.extension().substr(1 /* ignore dot */))) | ||||||
{ | ||||||
auto contents = fs.read_contents(path, VCPKG_LINE_INFO); | ||||||
files_and_contents.emplace_back(std::move(path), std::move(contents)); | ||||||
} | ||||||
} | ||||||
std::vector<std::string> string_paths; | ||||||
for (const auto& path : absolute_paths) | ||||||
{ | ||||||
#if defined(_WIN32) | ||||||
auto path_preferred = path; | ||||||
path_preferred.make_preferred(); | ||||||
string_paths.push_back(path_preferred.generic_u8string()); | ||||||
string_paths.push_back(std::move(path).native()); | ||||||
autoantwort marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
#else | ||||||
string_paths.push_back(path.native()); | ||||||
#endif | ||||||
} | ||||||
|
||||||
std::string result; | ||||||
for (const auto& path_and_contents : files_and_contents) | ||||||
{ | ||||||
const auto extension = path_and_contents.first.extension().substr(1 /* ignore dot */); | ||||||
const bool is_header = extension == "h" || extension == "hpp" || extension == "hxx"; | ||||||
vicroms marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if (Util::any_of(string_paths, [&path_and_contents, extension, is_header](const std::string& path) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Would be nice if we could use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That won't do anything, the input is already a |
||||||
if (extension == "cfg" || extension == "conf") | ||||||
{ | ||||||
return Strings::contains(path_and_contents.second, path); | ||||||
} | ||||||
for (size_t offset = 0;;) | ||||||
{ | ||||||
const auto index = path_and_contents.second.find(path, offset); | ||||||
if (index == std::string::npos) return false; | ||||||
if (is_header) | ||||||
{ | ||||||
bool new_line = false; | ||||||
for (auto start = index;;) | ||||||
{ | ||||||
const auto before = path_and_contents.second.find_last_of("\n/", start); | ||||||
if (before == std::string::npos) return true; | ||||||
if (path_and_contents.second[before] == '\n') | ||||||
{ | ||||||
if (before == 0) return true; | ||||||
new_line = true; | ||||||
start = before - 1; | ||||||
continue; | ||||||
} | ||||||
if (path_and_contents.second[before + 1] == '*') break; // is in a comment | ||||||
if (before == 0) return true; // not in a comment | ||||||
if (!new_line && path_and_contents.second[before - 1] == '/') break; // is in a comment | ||||||
if (path_and_contents.second[before - 1] == '*') return true; // is not in a comment | ||||||
start = before - 1; | ||||||
} | ||||||
} | ||||||
else | ||||||
{ // .py, .sh, .cmake or .pc file | ||||||
const auto before = path_and_contents.second.find_last_of("\n#", index); | ||||||
if (before == std::string::npos) return true; | ||||||
if (path_and_contents.second[before] == '\n') return true; // not a comment | ||||||
} | ||||||
offset = index + path.size(); | ||||||
} | ||||||
})) | ||||||
{ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. given that this is a warning, not a hard error, I'd prefer we just checked for the absolute paths without trying to look for whether we're in a comment. This is just a lot of complexity for little gain, imo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is a hard error in the ci... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right but we can fix those issues. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could, but this is a lot of work for nothing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add tests at least for the part that looks for absolute paths in header files. |
||||||
result += "\n "; | ||||||
result += path_and_contents.first.native(); | ||||||
} | ||||||
} | ||||||
|
||||||
if (!result.empty()) | ||||||
{ | ||||||
vicroms marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
msg::print(Color::warning, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would change the effects; I would prefer to merge this as is but I think we would welcome a following PR that changes all the post build checks to conform with our warnings guidelines. (Although @JavierMatosD may already have such a PR outstanding...) |
||||||
msg::format(msgFilesContainAbsolutePath, | ||||||
msg::absolute_paths = Strings::join("', '", absolute_paths), | ||||||
msg::paths = result) | ||||||
.append_raw("\n\n")); | ||||||
return LintStatus::PROBLEM_DETECTED; | ||||||
} | ||||||
return LintStatus::SUCCESS; | ||||||
} | ||||||
|
||||||
static void operator+=(size_t& left, const LintStatus& right) { left += static_cast<size_t>(right); } | ||||||
|
||||||
static size_t perform_all_checks_and_return_error_count(const PackageSpec& spec, | ||||||
|
@@ -1153,6 +1250,10 @@ namespace vcpkg::PostBuildLint | |||||
error_count += check_no_files_in_dir(fs, package_dir); | ||||||
error_count += check_no_files_in_dir(fs, package_dir / "debug"); | ||||||
error_count += check_pkgconfig_dir_only_in_lib_dir(fs, package_dir); | ||||||
error_count += check_no_absolute_paths_in( | ||||||
fs, | ||||||
package_dir, | ||||||
std::vector<Path>{package_dir.native(), paths.installed().root().native(), paths.build_dir(spec)}); | ||||||
|
||||||
return error_count; | ||||||
} | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to tell the user about the policy now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the policy should only be used internally to that merging this PR does the break the world. Imho we should not tell the users how to ignore warnings that warns before broken packages.