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

fix #12771: Progress value is not increased #6993

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
245636f
fix #12771
ludviggunne Nov 10, 2024
c35fd05
fix #12771 for ThreadExecutor
ludviggunne Nov 11, 2024
53d7585
use _stati64 on 64 bit windows
ludviggunne Nov 11, 2024
b25503d
move (most of the) file size logic to FileWithDetails
ludviggunne Nov 11, 2024
09936ca
use accumulate
ludviggunne Nov 11, 2024
4571588
update dmake dependencies
ludviggunne Nov 11, 2024
8129131
add const
ludviggunne Nov 11, 2024
8f5df68
remove unused includes
ludviggunne Nov 11, 2024
8cd166d
add test
ludviggunne Nov 14, 2024
3933832
move test
ludviggunne Nov 14, 2024
037c6ad
rebase
ludviggunne Nov 18, 2024
037cffe
fix path
ludviggunne Nov 18, 2024
3b7ed50
calculate percentage in test
ludviggunne Nov 18, 2024
11f7685
account for different file orders
ludviggunne Nov 19, 2024
5a97406
account for spaces in filenames
ludviggunne Nov 19, 2024
758b7ed
move filesize logic to path.cpp
ludviggunne Dec 22, 2024
4dc6684
set file sizes in CmdLineParser::fillSettingsFromArgs
ludviggunne Dec 22, 2024
ccad3f0
store filesize in pipeFile map
ludviggunne Dec 22, 2024
456de96
remove -j2 in progress test
ludviggunne Dec 22, 2024
2e09d44
update dmake
ludviggunne Dec 22, 2024
46e2a80
don't use ssize_t
ludviggunne Dec 22, 2024
006c9f4
fix typo
ludviggunne Dec 22, 2024
cd4ff8a
base percentage on file size in SingleExecutor
ludviggunne Dec 22, 2024
a9570ae
add test for .cppcheck files
ludviggunne Dec 26, 2024
feb624b
Update test/cli/proj2_test.py
ludviggunne Jan 3, 2025
c3a9e9c
fix FileWithDetails constructor
ludviggunne Jan 3, 2025
2ddba93
add test for files specified on command line
ludviggunne Jan 4, 2025
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
6 changes: 6 additions & 0 deletions cli/cmdlineparser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,9 @@ bool CmdLineParser::fillSettingsFromArgs(int argc, const char* const argv[])
if (mSettings.library.markupFile(fs.filename())) {
fs.file.setLang(Standards::Language::C);
}
std::string error = fs.updateFileSize();
if (!error.empty())
mLogger.printError(error);
Copy link
Owner

Choose a reason for hiding this comment

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

if you use mLogger.printError then it feels like we should abort. But I think it's unfortunate to abort if there are missing files. Could we write some proper cppcheck warnings during the analysis instead if a file cannot be opened.

How about just setting filesize to -1 or something:

  • when total file size is calculated ignore files that have size -1
  • during analysis ensure there is some proper cppcheck message when a file can not be opened.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose the files can just be removed from the list (after showing a warning)? That way we don't have to care about it when calculating the total file size.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If that error happens we should bail out - otherwise we would silently (nobody cares about the warnings because they are not visible in a headless system - yet) drop a file from the analysis which might cause issues to slip through or lead to inconsistent results between runs.

This should only happen if there are filesystem issues or somebody is modifying the data. You should avoid that. e.g. in the CLion IDE integration I pass a fixed temporary file to the analysis to avoid that a file which might constantly being changed is being read.

Copy link
Owner

Choose a reason for hiding this comment

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

If mLogger.printError(error); is used and Cppcheck just aborts then I see the risk that Cppcheck plugins will silently just fail and the user get no proper information. For instance the vscode plugin does not show the Cppcheck printError output it just does not recognize its output and aborts. It just says "Failed to execute Cppcheck" or something like that.

nobody cares about the warnings because they are not visible in a headless system - yet

I don't know what you talk about. It would be pointless to run Cppcheck at all if nobody cares about the warnings. I want that we report proper Cppcheck warning output so the plugins have a good chance to show the output.

This should only happen if there are filesystem issues or somebody is modifying the data.

Yeah sure.. but we do run into this sometimes. And it would be nice to be able to scan a project even if some files are autogenerated during build or whatever..

}

// sort the markup last
Expand Down Expand Up @@ -350,6 +353,9 @@ bool CmdLineParser::fillSettingsFromArgs(int argc, const char* const argv[])
if (mSettings.library.markupFile(f.path())) {
f.setLang(Standards::Language::C);
}
std::string error = f.updateSize();
if (!error.empty())
mLogger.printError(error);
}

// sort the markup last
Expand Down
8 changes: 4 additions & 4 deletions cli/filelister.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -232,11 +232,11 @@ static std::string addFiles2(std::list<FileWithDetails> &files,
} else {
Standards::Language lang = Standards::Language::None;
if (Path::acceptFile(new_path, extra, &lang) && !ignored.match(new_path)) {
if (stat(new_path.c_str(), &file_stat) == -1) {
const int err = errno;
return "could not stat file '" + new_path + "' (errno: " + std::to_string(err) + ")";
files.emplace_back(new_path, lang);
std::string err = files.back().updateSize();
if (!err.empty()) {
return err;
}
files.emplace_back(new_path, lang, file_stat.st_size);
}
}
}
Expand Down
25 changes: 11 additions & 14 deletions cli/processexecutor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -239,13 +239,15 @@ unsigned int ProcessExecutor::check()
unsigned int fileCount = 0;
unsigned int result = 0;

const std::size_t totalfilesize = std::accumulate(mFiles.cbegin(), mFiles.cend(), std::size_t(0), [](std::size_t v, const FileWithDetails& p) {
return v + p.size();
const std::size_t totalfilesize = std::accumulate(mFiles.cbegin(), mFiles.cend(), std::size_t(0), [](std::size_t v, const FileWithDetails& fwd) {
return v + fwd.size();
}) + std::accumulate(mFileSettings.cbegin(), mFileSettings.cend(), std::size_t(0), [](std::size_t v, const FileSettings& fs) {
return v + fs.filesize();
});

std::list<int> rpipes;
std::map<pid_t, std::string> childFile;
std::map<int, std::string> pipeFile;
std::map<int, std::pair<std::string, std::size_t>> pipeFile;
std::size_t processedsize = 0;
auto iFile = mFiles.cbegin();
auto iFileSettings = mFileSettings.cbegin();
Expand Down Expand Up @@ -304,11 +306,11 @@ unsigned int ProcessExecutor::check()
rpipes.push_back(pipes[0]);
if (iFileSettings != mFileSettings.end()) {
childFile[pid] = iFileSettings->filename() + ' ' + iFileSettings->cfg;
pipeFile[pipes[0]] = iFileSettings->filename() + ' ' + iFileSettings->cfg;
pipeFile[pipes[0]] = { iFileSettings->filename() + ' ' + iFileSettings->cfg, iFileSettings->filesize() };
++iFileSettings;
} else {
childFile[pid] = iFile->path();
pipeFile[pipes[0]] = iFile->path();
pipeFile[pipes[0]] = { iFile->path(), iFile->size() };
++iFile;
}
}
Expand All @@ -327,21 +329,16 @@ unsigned int ProcessExecutor::check()
while (rp != rpipes.cend()) {
if (FD_ISSET(*rp, &rfds)) {
std::string name;
const auto p = utils::as_const(pipeFile).find(*rp);
std::size_t size = 0;
const std::map<int, std::pair<std::string, std::size_t>>::const_iterator p = pipeFile.find(*rp);
if (p != pipeFile.cend()) {
name = p->second;
name = p->second.first;
size = p->second.second;
}
const bool readRes = handleRead(*rp, result, name);
if (!readRes) {
std::size_t size = 0;
if (p != pipeFile.cend()) {
pipeFile.erase(p);
const auto fs = std::find_if(mFiles.cbegin(), mFiles.cend(), [&name](const FileWithDetails& entry) {
return entry.path() == name;
});
if (fs != mFiles.end()) {
size = fs->size();
}
}

fileCount++;
Expand Down
5 changes: 4 additions & 1 deletion cli/singleexecutor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ unsigned int SingleExecutor::check()

const std::size_t totalfilesize = std::accumulate(mFiles.cbegin(), mFiles.cend(), std::size_t(0), [](std::size_t v, const FileWithDetails& f) {
return v + f.size();
}) + std::accumulate(mFileSettings.cbegin(), mFileSettings.cend(), std::size_t(0), [](std::size_t v, const FileSettings& fs) {
return v + fs.filesize();
});

std::size_t processedsize = 0;
Expand All @@ -62,9 +64,10 @@ unsigned int SingleExecutor::check()
// check all files of the project
for (const FileSettings &fs : mFileSettings) {
result += mCppcheck.check(fs);
processedsize += fs.filesize();
++c;
if (!mSettings.quiet)
reportStatus(c, mFileSettings.size(), c, mFileSettings.size());
reportStatus(c, mFileSettings.size(), processedsize, totalfilesize);
if (mSettings.clangTidy)
mCppcheck.analyseClangTidy(fs);
}
Expand Down
4 changes: 3 additions & 1 deletion cli/threadexecutor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ class ThreadData
mTotalFiles = mFiles.size() + mFileSettings.size();
mTotalFileSize = std::accumulate(mFiles.cbegin(), mFiles.cend(), std::size_t(0), [](std::size_t v, const FileWithDetails& p) {
return v + p.size();
}) + std::accumulate(mFileSettings.cbegin(), mFileSettings.cend(), std::size_t(0), [](std::size_t v, const FileSettings& p) {
return v + p.filesize();
});
}

Expand All @@ -104,7 +106,7 @@ class ThreadData
if (mItNextFileSettings != mFileSettings.end()) {
file = nullptr;
fs = &(*mItNextFileSettings);
fileSize = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make sure we always fetch the filesize we probably should initialize/set it to -1 everywhere and valid that later on. Can be done at a later date though.

fileSize = mItNextFileSettings->filesize();
++mItNextFileSettings;
return true;
}
Expand Down
30 changes: 30 additions & 0 deletions lib/filesettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@ class FileWithDetails
throw std::runtime_error("empty path specified");
}

FileWithDetails(std::string path, Standards::Language lang)
: mPath(std::move(path))
, mPathSimplified(Path::simplifyPath(mPath))
, mLang(lang)
, mSize(0)
{
if (mPath.empty())
throw std::runtime_error("empty path specified");
}

const std::string& path() const
{
return mPath;
Expand All @@ -71,6 +81,16 @@ class FileWithDetails
{
return mLang;
}

std::string updateSize()
ludviggunne marked this conversation as resolved.
Show resolved Hide resolved
{
long long ssize = Path::fileSize(mPath);
if (ssize < 0)
return "could not stat file '" + mPath + "': (errno: " + std::to_string(errno) + ")";
mSize = ssize;
return "";
}

private:
std::string mPath;
std::string mPathSimplified;
Expand Down Expand Up @@ -99,6 +119,16 @@ struct CPPCHECKLIB FileSettings {
{
return file.spath();
}
std::size_t filesize() const
{
return file.size();
}

std::string updateFileSize()
{
return file.updateSize();
}

std::string defines;
// TODO: handle differently
std::string cppcheckDefines() const {
Expand Down
1 change: 1 addition & 0 deletions lib/importproject.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ class CPPCHECKLIB WARN_UNUSED ImportProject {
static void printError(const std::string &message);

void setRelativePaths(const std::string &filename);
void setFileSizes();

std::string mPath;
std::set<std::string> mAllVSConfigs;
Expand Down
36 changes: 36 additions & 0 deletions lib/path.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -439,3 +439,39 @@ std::string Path::join(const std::string& path1, const std::string& path2) {
return path2;
return ((path1.back() == '/') ? path1 : (path1 + "/")) + path2;
}

#ifdef _WIN32

# ifdef _WIN64

long long Path::fileSize(const std::string &filePath) {
struct _stati64 buf;
if (_stati64(filePath.c_str(), &buf) < 0) {
return -1;
}
return buf.st_size;
}

# else

long long Path::fileSize(const std::string &filePath) {
struct _stat buf;
if (_stat(filePath.c_str(), &buf) < 0) {
return -1;
}
return buf.st_size;
}

# endif

#else

long long Path::fileSize(const std::string &filePath) {
struct stat buf;
if (stat(filePath.c_str(), &buf) < 0) {
return -1;
}
return buf.st_size;
}

#endif
7 changes: 7 additions & 0 deletions lib/path.h
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,13 @@ class CPPCHECKLIB Path {
* join 2 paths with '/' separators
*/
static std::string join(const std::string& path1, const std::string& path2);

/**
* @brief Get the size of a file
* @param filePath path to the file
* @return size of file, or -1 if the file cannot be accessed
*/
static long long fileSize(const std::string &filePath);
};

/// @}
Expand Down
40 changes: 40 additions & 0 deletions test/cli/proj2_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,46 @@ def test_absolute_path(tmp_path):
assert stdout.find('Checking %s ...' % file1) >= 0
assert stdout.find('Checking %s ...' % file2) >= 0

def test_progress_json(tmp_path):
proj_dir = tmp_path / 'proj2'
shutil.copytree(__proj_dir, proj_dir)
size1 = os.path.getsize(os.path.join(proj_dir, 'a', 'a.c'))
size2 = os.path.getsize(os.path.join(proj_dir, 'b', 'b.c'))
perc1 = 100 * size1 // (size1 + size2)
perc2 = 100 * size2 // (size1 + size2)
__create_compile_commands(proj_dir)
ret, stdout, _ = cppcheck(['--project=' + os.path.join(proj_dir, __COMPILE_COMMANDS_JSON)], cwd=tmp_path)
assert ret == 0, stdout
assert stdout.find('1/2 files checked %d%% done' % perc1) >= 0 or stdout.find('1/2 files checked %d%% done' % perc2) >= 0
assert stdout.find('2/2 files checked 100% done') >= 0

def test_progress_cppcheck(tmp_path):
proj_dir = tmp_path / 'proj2'
shutil.copytree(__proj_dir, proj_dir)
size1 = os.path.getsize(os.path.join(proj_dir, 'a', 'a.c'))
size2 = os.path.getsize(os.path.join(proj_dir, 'b', 'b.c'))
perc1 = 100 * size1 // (size1 + size2)
perc2 = 100 * size2 // (size1 + size2)
__create_compile_commands(proj_dir)
ret, stdout, _ = cppcheck(['--project=proj2/proj2.cppcheck'], cwd=tmp_path)
assert ret == 0, stdout
assert stdout.find('1/2 files checked %d%% done' % perc1) >= 0 or stdout.find('1/2 files checked %d%% done' % perc2) >= 0
assert stdout.find('2/2 files checked 100% done') >= 0

def test_progress_cli(tmp_path):
proj_dir = tmp_path / 'proj2'
shutil.copytree(__proj_dir, proj_dir)
path1 = os.path.join(proj_dir, 'a', 'a.c')
path2 = os.path.join(proj_dir, 'b', 'b.c')
size1 = os.path.getsize(path1)
size2 = os.path.getsize(path2)
perc1 = 100 * size1 // (size1 + size2)
perc2 = 100 * size2 // (size1 + size2)
ret, stdout, _ = cppcheck([path1, path2], cwd=tmp_path)
assert ret == 0, stdout
assert stdout.find('1/2 files checked %d%% done' % perc1) >= 0 or stdout.find('1/2 files checked %d%% done' % perc2) >= 0
assert stdout.find('2/2 files checked 100% done') >= 0

def test_gui_project_loads_compile_commands_1(tmp_path):
proj_dir = tmp_path / 'proj2'
shutil.copytree(__proj_dir, proj_dir)
Expand Down
Loading