diff --git a/apps/librepcb-cli/commandlineinterface.cpp b/apps/librepcb-cli/commandlineinterface.cpp index 58b684123e..10c11dbe5c 100644 --- a/apps/librepcb-cli/commandlineinterface.cpp +++ b/apps/librepcb-cli/commandlineinterface.cpp @@ -131,6 +131,10 @@ int CommandLineInterface::execute() noexcept { QCommandLineOption saveOption( "save", tr("Save project before closing it (useful to upgrade file format).")); + QCommandLineOption prjStrictOption( + "strict", tr("Fail if the project files are not strictly canonical, i.e. " + "there would be changes when saving the project. Note that " + "this option is not available for *.lppz files.")); // Define options for "open-library" QCommandLineOption libAllOption( @@ -139,6 +143,9 @@ int CommandLineInterface::execute() noexcept { QCommandLineOption libSaveOption( "save", tr("Save library (and contained elements if '--all' is given) " "before closing them (useful to upgrade file format).")); + QCommandLineOption libStrictOption( + "strict", tr("Fail if the opened files are not strictly canonical, i.e. " + "there would be changes when saving the library elements.")); // First parse to get the supplied command (ignoring errors because the parser // does not yet know the command-dependent options). @@ -166,6 +173,7 @@ int CommandLineInterface::execute() noexcept { parser.addOption(pcbFabricationSettingsOption); parser.addOption(boardOption); parser.addOption(saveOption); + parser.addOption(prjStrictOption); } else if (command == "open-library") { parser.clearPositionalArguments(); parser.addPositionalArgument(command, commands[command].first, @@ -174,6 +182,7 @@ int CommandLineInterface::execute() noexcept { tr("Path to library directory (*.lplib).")); parser.addOption(libAllOption); parser.addOption(libSaveOption); + parser.addOption(libStrictOption); } else if (!command.isEmpty()) { printErr(QString(tr("Unknown command '%1'.")).arg(command), 2); print(parser.helpText(), 0); @@ -234,7 +243,8 @@ int CommandLineInterface::execute() noexcept { parser.isSet(exportPcbFabricationDataOption), // export PCB fab. data parser.value(pcbFabricationSettingsOption), // PCB fab. settings parser.values(boardOption), // boards - parser.isSet(saveOption) // save project + parser.isSet(saveOption), // save project + parser.isSet(prjStrictOption) // strict mode ); } else if (command == "open-library") { if (positionalArgs.count() != 1) { @@ -242,9 +252,10 @@ int CommandLineInterface::execute() noexcept { print(parser.helpText(), 0); return 1; } - cmdSuccess = openLibrary(positionalArgs.value(0), // library directory - parser.isSet(libAllOption), // all elements - parser.isSet(libSaveOption) // save + cmdSuccess = openLibrary(positionalArgs.value(0), // library directory + parser.isSet(libAllOption), // all elements + parser.isSet(libSaveOption), // save + parser.isSet(libStrictOption) // strict mode ); } else { printErr(tr("Internal failure.")); @@ -267,7 +278,7 @@ bool CommandLineInterface::openProject( const QStringList& exportSchematicsFiles, const QStringList& exportBomFiles, const QStringList& exportBoardBomFiles, const QString& bomAttributes, bool exportPcbFabricationData, const QString& pcbFabricationSettingsPath, - const QStringList& boards, bool save) const noexcept { + const QStringList& boards, bool save, bool strict) const noexcept { try { bool success = true; QMap writtenFilesCounter; @@ -295,6 +306,31 @@ bool CommandLineInterface::openProject( new TransactionalDirectory(projectFs)), projectFileName); // can throw + // Check for non-canonical files (strict mode) + if (strict) { + print(tr("Check for non-canonical files...")); + if (projectFp.getSuffix() == "lppz") { + printErr(" " % tr("ERROR: The option '--strict' is not available for " + "*.lppz files!")); + success = false; + } else { + project.save(); // can throw + QStringList paths = projectFs->checkForModifications(); // can throw + // ignore user config files + paths = paths.filter(QRegularExpression("^((?!\\.user\\.lp).)*$")); + // sort file paths to increases readability of console output + std::sort(paths.begin(), paths.end()); + foreach (const QString& path, paths) { + printErr( + QString(" - Non-canonical file: %1") + .arg(prettyPath(projectFs->getAbsPath(path), projectFile))); + } + if (paths.count() > 0) { + success = false; + } + } + } + // ERC if (runErc) { print(tr("Run ERC...")); @@ -501,7 +537,7 @@ bool CommandLineInterface::openProject( } bool CommandLineInterface::openLibrary(const QString& libDir, bool all, - bool save) const noexcept { + bool save, bool strict) const noexcept { try { bool success = true; @@ -513,6 +549,8 @@ bool CommandLineInterface::openLibrary(const QString& libDir, bool all, TransactionalFileSystem::open(libFp, save); // can throw Library lib(std::unique_ptr( new TransactionalDirectory(libFs))); // can throw + processLibraryElement(libDir, *libFs, lib, save, strict, + success); // can throw // Open all component categories if (all) { @@ -526,11 +564,8 @@ bool CommandLineInterface::openLibrary(const QString& libDir, bool all, TransactionalFileSystem::open(fp, save); // can throw ComponentCategory element(std::unique_ptr( new TransactionalDirectory(fs))); // can throw - if (save) { - qInfo() << QString(tr("Save '%1'...")).arg(prettyPath(fp, libDir)); - element.save(); // can throw - fs->save(); // can throw - } + processLibraryElement(libDir, *fs, element, save, strict, + success); // can throw } } @@ -546,11 +581,8 @@ bool CommandLineInterface::openLibrary(const QString& libDir, bool all, TransactionalFileSystem::open(fp, save); // can throw PackageCategory element(std::unique_ptr( new TransactionalDirectory(fs))); // can throw - if (save) { - qInfo() << QString(tr("Save '%1'...")).arg(prettyPath(fp, libDir)); - element.save(); // can throw - fs->save(); // can throw - } + processLibraryElement(libDir, *fs, element, save, strict, + success); // can throw } } @@ -565,11 +597,8 @@ bool CommandLineInterface::openLibrary(const QString& libDir, bool all, TransactionalFileSystem::open(fp, save); // can throw Symbol element(std::unique_ptr( new TransactionalDirectory(fs))); // can throw - if (save) { - qInfo() << QString(tr("Save '%1'...")).arg(prettyPath(fp, libDir)); - element.save(); // can throw - fs->save(); // can throw - } + processLibraryElement(libDir, *fs, element, save, strict, + success); // can throw } } @@ -584,11 +613,8 @@ bool CommandLineInterface::openLibrary(const QString& libDir, bool all, TransactionalFileSystem::open(fp, save); // can throw Package element(std::unique_ptr( new TransactionalDirectory(fs))); // can throw - if (save) { - qInfo() << QString(tr("Save '%1'...")).arg(prettyPath(fp, libDir)); - element.save(); // can throw - fs->save(); // can throw - } + processLibraryElement(libDir, *fs, element, save, strict, + success); // can throw } } @@ -603,11 +629,8 @@ bool CommandLineInterface::openLibrary(const QString& libDir, bool all, TransactionalFileSystem::open(fp, save); // can throw Component element(std::unique_ptr( new TransactionalDirectory(fs))); // can throw - if (save) { - qInfo() << QString(tr("Save '%1'...")).arg(prettyPath(fp, libDir)); - element.save(); // can throw - fs->save(); // can throw - } + processLibraryElement(libDir, *fs, element, save, strict, + success); // can throw } } @@ -622,21 +645,11 @@ bool CommandLineInterface::openLibrary(const QString& libDir, bool all, TransactionalFileSystem::open(fp, save); // can throw Device element(std::unique_ptr( new TransactionalDirectory(fs))); // can throw - if (save) { - qInfo() << QString(tr("Save '%1'...")).arg(prettyPath(fp, libDir)); - element.save(); // can throw - fs->save(); // can throw - } + processLibraryElement(libDir, *fs, element, save, strict, + success); // can throw } } - // Save library - if (save) { - print(QString(tr("Save library '%1'...")).arg(prettyPath(libFp, libDir))); - lib.save(); // can throw - libFs->save(); // can throw - } - return success; } catch (const Exception& e) { printErr(QString(tr("ERROR: %1")).arg(e.getMsg())); @@ -644,6 +657,45 @@ bool CommandLineInterface::openLibrary(const QString& libDir, bool all, } } +void CommandLineInterface::processLibraryElement(const QString& libDir, + TransactionalFileSystem& fs, + LibraryBaseElement& element, + bool save, bool strict, + bool& success) const { + // Save element to transactional file system, if needed + if (strict || save) { + element.save(); // can throw + } + + // Check for non-canonical files (strict mode) + if (strict) { + qInfo() << QString(tr("Check '%1' for non-canonical files...")) + .arg(prettyPath(fs.getPath(), libDir)); + + QStringList paths = fs.checkForModifications(); // can throw + // sort file paths to increases readability of console output + std::sort(paths.begin(), paths.end()); + foreach (const QString& path, paths) { + printErr(QString(" - Non-canonical file: %1") + .arg(prettyPath(fs.getAbsPath(path), libDir))); + } + if (paths.count() > 0) { + success = false; + } + } + + // Save element to file system, if needed + if (save) { + qInfo() + << QString(tr("Save '%1'...")).arg(prettyPath(fs.getPath(), libDir)); + fs.save(); // can throw + } + + // Do not propagate changes in the transactional file system to the + // following checks + fs.discardChanges(); +} + QString CommandLineInterface::prettyPath(const FilePath& path, const QString& style) noexcept { if (QFileInfo(style).isAbsolute()) { diff --git a/apps/librepcb-cli/commandlineinterface.h b/apps/librepcb-cli/commandlineinterface.h index 012dcfbf93..0b1086a596 100644 --- a/apps/librepcb-cli/commandlineinterface.h +++ b/apps/librepcb-cli/commandlineinterface.h @@ -32,6 +32,11 @@ namespace librepcb { class Application; class FilePath; +class TransactionalFileSystem; + +namespace library { +class LibraryBaseElement; +} namespace cli { @@ -61,8 +66,13 @@ class CommandLineInterface final { const QStringList& exportBoardBomFiles, const QString& bomAttributes, bool exportPcbFabricationData, const QString& pcbFabricationSettingsPath, - const QStringList& boards, bool save) const noexcept; - bool openLibrary(const QString& libDir, bool all, bool save) const noexcept; + const QStringList& boards, bool save, bool strict) const + noexcept; + bool openLibrary(const QString& libDir, bool all, bool save, + bool strict) const noexcept; + void processLibraryElement(const QString& libDir, TransactionalFileSystem& fs, + library::LibraryBaseElement& element, bool save, + bool strict, bool& success) const; static QString prettyPath(const FilePath& path, const QString& style) noexcept; static void print(const QString& str, int newlines = 1) noexcept; diff --git a/libs/librepcb/common/fileio/transactionalfilesystem.cpp b/libs/librepcb/common/fileio/transactionalfilesystem.cpp index 96b30c476c..e5f3acc045 100644 --- a/libs/librepcb/common/fileio/transactionalfilesystem.cpp +++ b/libs/librepcb/common/fileio/transactionalfilesystem.cpp @@ -267,6 +267,44 @@ void TransactionalFileSystem::exportToZip(const FilePath& fp) const { } } +void TransactionalFileSystem::discardChanges() noexcept { + mModifiedFiles.clear(); + mRemovedFiles.clear(); + mRemovedDirs.clear(); +} + +QStringList TransactionalFileSystem::checkForModifications() const { + QStringList modifications; + + // removed directories + foreach (const QString& dir, mRemovedDirs) { + FilePath fp = mFilePath.getPathTo(dir); + if (fp.isExistingDir()) { + modifications.append(dir); + } + } + + // removed files + foreach (const QString& filepath, mRemovedFiles) { + FilePath fp = mFilePath.getPathTo(filepath); + if (fp.isExistingFile()) { + modifications.append(filepath); + } + } + + // new or modified files + foreach (const QString& filepath, mModifiedFiles.keys()) { + FilePath fp = mFilePath.getPathTo(filepath); + QByteArray content = mModifiedFiles.value(filepath); + if ((!fp.isExistingFile()) || + (FileUtils::readFile(fp) != content)) { // can throw + modifications.append(filepath); + } + } + + return modifications; +} + void TransactionalFileSystem::autosave() { saveDiff("autosave"); // can throw } @@ -449,12 +487,6 @@ void TransactionalFileSystem::removeDiff(const QString& type) { FileUtils::removeDirRecursively(dir); // can throw } -void TransactionalFileSystem::discardChanges() noexcept { - mModifiedFiles.clear(); - mRemovedFiles.clear(); - mRemovedDirs.clear(); -} - /******************************************************************************* * End of File ******************************************************************************/ diff --git a/libs/librepcb/common/fileio/transactionalfilesystem.h b/libs/librepcb/common/fileio/transactionalfilesystem.h index d7c516379d..33ca957c08 100644 --- a/libs/librepcb/common/fileio/transactionalfilesystem.h +++ b/libs/librepcb/common/fileio/transactionalfilesystem.h @@ -80,7 +80,8 @@ class TransactionalFileSystem final : public FileSystem { virtual ~TransactionalFileSystem() noexcept; // Getters - bool isWritable() const noexcept { return mIsWritable; } + const FilePath& getPath() const noexcept { return mFilePath; } + bool isWritable() const noexcept { return mIsWritable; } bool isRestoredFromAutosave() const noexcept { return mRestoredFromAutosave; } // Inherited from FileSystem @@ -95,10 +96,12 @@ class TransactionalFileSystem final : public FileSystem { virtual void removeDirRecursively(const QString& path = "") override; // General Methods - void loadFromZip(const FilePath& fp); - void exportToZip(const FilePath& fp) const; - void autosave(); - void save(); + void loadFromZip(const FilePath& fp); + void exportToZip(const FilePath& fp) const; + void discardChanges() noexcept; + QStringList checkForModifications() const; + void autosave(); + void save(); // Static Methods static std::shared_ptr open( @@ -126,7 +129,6 @@ class TransactionalFileSystem final : public FileSystem { void saveDiff(const QString& type) const; void loadDiff(const FilePath& fp); void removeDiff(const QString& type); - void discardChanges() noexcept; private: // Data FilePath mFilePath; diff --git a/tests/cli/open-project/test_save.py b/tests/cli/open-project/test_save.py index 5dfdec32dc..da6fc81ec0 100644 --- a/tests/cli/open-project/test_save.py +++ b/tests/cli/open-project/test_save.py @@ -9,9 +9,6 @@ Test command "open-project --save" """ -PROJECT_LPP = 'data/Empty Project/Empty Project.lpp' -PROJECT_LPPZ = 'data/Empty Project.lppz' - @pytest.mark.parametrize("project", [ params.EMPTY_PROJECT_LPP_PARAM, diff --git a/tests/cli/open-project/test_strict.py b/tests/cli/open-project/test_strict.py new file mode 100644 index 0000000000..b98c95eda5 --- /dev/null +++ b/tests/cli/open-project/test_strict.py @@ -0,0 +1,45 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- + +import params + +""" +Test command "open-project --strict" +""" + + +def test_valid_lpp(cli): + project = params.EMPTY_PROJECT_LPP + cli.add_project(project.dir, as_lppz=project.is_lppz) + code, stdout, stderr = cli.run('open-project', '--strict', project.path) + assert code == 0 + assert len(stderr) == 0 + assert len(stdout) > 0 + assert stdout[-1] == 'SUCCESS' + + +def test_invalid_lpp(cli): + project = params.EMPTY_PROJECT_LPP + cli.add_project(project.dir, as_lppz=project.is_lppz) + # append some zeros to the project file + path = cli.abspath(project.path) + with open(path, 'ab') as f: + f.write(b'\0\0') + # open project + code, stdout, stderr = cli.run('open-project', '--strict', project.path) + assert code == 1 + assert len(stderr) == 1 + assert 'Non-canonical file:' in stderr[0] + assert len(stdout) > 0 + assert stdout[-1] == 'Finished with errors!' + + +def test_lppz_fails(cli): + project = params.PROJECT_WITH_TWO_BOARDS_LPPZ + cli.add_project(project.dir, as_lppz=project.is_lppz) + code, stdout, stderr = cli.run('open-project', '--strict', project.path) + assert code == 1 + assert len(stderr) == 1 + assert "The option '--strict' is not available" in stderr[0] + assert len(stdout) > 0 + assert stdout[-1] == 'Finished with errors!' diff --git a/tests/unittests/common/fileio/transactionalfilesystemtest.cpp b/tests/unittests/common/fileio/transactionalfilesystemtest.cpp index c60bd0d30a..81bdef9725 100644 --- a/tests/unittests/common/fileio/transactionalfilesystemtest.cpp +++ b/tests/unittests/common/fileio/transactionalfilesystemtest.cpp @@ -92,6 +92,11 @@ TEST_F(TransactionalFileSystemTest, testConstructorPopulatedDir) { TransactionalFileSystem fs(mPopulatedDir, true); } +TEST_F(TransactionalFileSystemTest, testGetPath) { + TransactionalFileSystem fs(mPopulatedDir, false); + EXPECT_EQ(mPopulatedDir, fs.getPath()); +} + TEST_F(TransactionalFileSystemTest, testIsWritableFalse) { TransactionalFileSystem fs(mPopulatedDir, false); EXPECT_FALSE(fs.isWritable()); @@ -598,6 +603,103 @@ TEST_F(TransactionalFileSystemTest, testExportToZip) { EXPECT_TRUE(zipFp.isExistingFile()); } +TEST_F(TransactionalFileSystemTest, testDiscardChanges) { + TransactionalFileSystem fs(mPopulatedDir, true); + + // check initial state + ASSERT_FALSE(fs.fileExists("x/y/z")); + ASSERT_FALSE(fs.fileExists("z/y/x.txt")); + ASSERT_FALSE(fs.fileExists("z/y.txt")); + ASSERT_TRUE(fs.fileExists("1.txt")); + ASSERT_TRUE(fs.fileExists("a/b/c")); + ASSERT_FALSE(fs.fileExists("z/1.txt")); + ASSERT_FALSE(fs.fileExists("z/2.txt")); + + // do some file operations + fs.write("x/y/z", "z"); // create new file + fs.write("z/y/x.txt", "x"); // create new file + fs.write("z/y.txt", "y"); // create new file + fs.write("1.txt", "new 1"); // overwrite existing file + fs.write(".dot/file.txt", "new file"); // overwrite existing file + fs.removeFile("z/y/x.txt"); // remove new file + fs.removeFile("1.txt"); // remove existing file + fs.removeDirRecursively("z"); // remove new directory + fs.removeDirRecursively("a"); // remove existing directory + fs.write("z/1.txt", "1"); // create new file + fs.write("z/2.txt", "2"); // create new file + fs.removeFile("z/1.txt"); // remove new file + + // discard all changes + fs.discardChanges(); + + // check state in memory + EXPECT_FALSE(fs.fileExists("x/y/z")); + EXPECT_FALSE(fs.fileExists("z/y/x.txt")); + EXPECT_FALSE(fs.fileExists("z/y.txt")); + EXPECT_TRUE(fs.fileExists("1.txt")); + EXPECT_TRUE(fs.fileExists("a/b/c")); + EXPECT_FALSE(fs.fileExists("z/1.txt")); + EXPECT_FALSE(fs.fileExists("z/2.txt")); + + // save to file system + fs.save(); + + // check state on file system + EXPECT_FALSE(fs.getAbsPath("x/y/z").isExistingFile()); + EXPECT_FALSE(fs.getAbsPath("z/y/x.txt").isExistingFile()); + EXPECT_FALSE(fs.getAbsPath("z/y.txt").isExistingFile()); + EXPECT_TRUE(fs.getAbsPath("1.txt").isExistingFile()); + EXPECT_TRUE(fs.getAbsPath("a/b/c").isExistingFile()); + EXPECT_FALSE(fs.getAbsPath("z/1.txt").isExistingFile()); + EXPECT_FALSE(fs.getAbsPath("z/2.txt").isExistingFile()); + EXPECT_EQ("1", FileUtils::readFile(fs.getAbsPath("1.txt"))); + EXPECT_EQ("c", FileUtils::readFile(fs.getAbsPath("a/b/c"))); + EXPECT_EQ("file", FileUtils::readFile(fs.getAbsPath(".dot/file.txt"))); +} + +TEST_F(TransactionalFileSystemTest, testCheckForModifications) { + TransactionalFileSystem fs(mPopulatedDir, true); + + // check initial state + ASSERT_FALSE(fs.fileExists("x/y/z")); + ASSERT_FALSE(fs.fileExists("z/y/x.txt")); + ASSERT_FALSE(fs.fileExists("z/y.txt")); + ASSERT_TRUE(fs.fileExists("1.txt")); + ASSERT_TRUE(fs.fileExists("a/b/c")); + ASSERT_FALSE(fs.fileExists("z/1.txt")); + ASSERT_FALSE(fs.fileExists("z/2.txt")); + + // do some file operations + fs.write("x/y/z", "z"); // create new file + fs.write("z/y/x.txt", "x"); // create new file + fs.write("z/y.txt", "y"); // create new file + fs.write("1.txt", "new 1"); // overwrite existing file + fs.write(".dot/file.txt", "new file"); // overwrite existing file + fs.removeFile("z/y/x.txt"); // remove new file + fs.removeFile("1.txt"); // remove existing file + fs.removeDirRecursively("z"); // remove new directory + fs.removeDirRecursively("a"); // remove existing directory + fs.write("z/1.txt", "1"); // create new file + fs.write("z/2.txt", "2"); // create new file + fs.removeFile("z/1.txt"); // remove new file + + // check modifications + std::list modified; + foreach (const QString& str, fs.checkForModifications()) { + modified.push_back(str.toStdString()); + } + modified.sort(); + std::list expected = {".dot/file.txt", "1.txt", "a/", "x/y/z", + "z/2.txt"}; + EXPECT_EQ(expected, modified); + + // save to file system + fs.save(); + + // check modifications, should be empty now + EXPECT_EQ(0, fs.checkForModifications().count()); +} + /******************************************************************************* * Parametrized getSubDirs() Tests ******************************************************************************/