From 77e8588b482ba3fdaffbbcd844e8a40227ad8476 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 2 Jan 2023 00:40:07 +0100 Subject: [PATCH] [wip] corrade-rc: allow shorthand filename specification. If no alias, nullTerminated or align options are needed or the global values are sufficient, having to write a [file] section for each file is overly verbose. This allows to specify filename= options directly in the global configuration section. It's filename= and not file= in order to not have to further complicate the CMake parsing routine that extract dependency information from the file -- this way it doesn't even need to be changed. TODO: while basic support for this was easy enough, the filesystem group override code needs to be adapted and tested for this, which involves copying the file somewhere temporary and changing the contents (because we can't make use of an alias in this scenario) TODO: there's also a newly uncovered bug, see the TODO in Resource.cpp TODO: this also needs to be documented --- .../Utility/Implementation/ResourceCompile.h | 21 +++++++++++++++++- src/Corrade/Utility/Resource.cpp | 22 +++++++++++++++++++ src/Corrade/Utility/Test/CMakeLists.txt | 2 ++ .../Utility/Test/ResourceCompileTest.cpp | 7 +++++- src/Corrade/Utility/Test/ResourceTest.cpp | 2 ++ .../resources-empty-file-filename.conf | 4 ++++ .../resources-empty-filename.conf | 2 +- .../resources-file-nonexistent.conf | 5 +++++ .../resources-nonexistent.conf | 3 --- .../resources-overridden.conf | 5 ++--- .../ResourceTestFiles/resources-spaces.conf | 2 -- .../Test/ResourceTestFiles/resources.conf | 3 +++ 12 files changed, 67 insertions(+), 11 deletions(-) create mode 100644 src/Corrade/Utility/Test/ResourceTestFiles/resources-empty-file-filename.conf create mode 100644 src/Corrade/Utility/Test/ResourceTestFiles/resources-file-nonexistent.conf diff --git a/src/Corrade/Utility/Implementation/ResourceCompile.h b/src/Corrade/Utility/Implementation/ResourceCompile.h index fff4f41d7..6783f757b 100644 --- a/src/Corrade/Utility/Implementation/ResourceCompile.h +++ b/src/Corrade/Utility/Implementation/ResourceCompile.h @@ -304,9 +304,28 @@ Containers::String resourceCompileFrom(const Containers::StringView name, const } /* Load all files */ + std::vector filenames = conf.values("filename"); std::vector files = conf.groups("file"); Containers::Array fileData; - arrayReserve(fileData, files.size()); + arrayReserve(fileData, filenames.size() + files.size()); + + /* Process loose filename options -- they have no aliases and always + inherit the global options */ + for(const Containers::StringView filename: filenames) { + if(filename.isEmpty()) { + Error() << " Error: filename" << fileData.size() + 1 << "in group" << group << "is empty"; + return {}; + } + + Containers::Optional> contents = Path::read(Path::join(path, filename)); + if(!contents) { + Error() << " Error: cannot open file" << filename << "of file" << fileData.size()+1 << "in group" << group; + return {}; + } + arrayAppend(fileData, InPlaceInit, filename, globalNullTerminated, globalAlign, *std::move(contents)); + } + + /* Process [file] groups */ for(const ConfigurationGroup* const file: files) { const Containers::StringView filename = file->value("filename"); const Containers::StringView alias = file->hasValue("alias") ? file->value("alias") : filename; diff --git a/src/Corrade/Utility/Resource.cpp b/src/Corrade/Utility/Resource.cpp index d1ebcad9b..bcdf56ecd 100644 --- a/src/Corrade/Utility/Resource.cpp +++ b/src/Corrade/Utility/Resource.cpp @@ -168,6 +168,7 @@ Resource::Resource(const Containers::StringView group): _group{findGroup(group)} << "Utility::Resource: group '" << Debug::nospace << group << Debug::nospace << "' overridden with '" << Debug::nospace << overridden->second << Debug::nospace << "\'"; _overrideGroup = new OverrideData(overridden->second); + // TODO this gets printed also if the overriden file didn't even exist, FFS if(_overrideGroup->conf.value("group") != group) Warning{} << "Utility::Resource: overridden with different group, found '" << Debug::nospace << _overrideGroup->conf.value("group") @@ -216,6 +217,27 @@ Containers::StringView Resource::getString(const Containers::StringView filename /* Load the file and save it for later use. Linear search is not an issue, as this shouldn't be used in production code anyway. */ + + /* Loose filenames first */ + const std::vector filenames = _overrideGroup->conf.values("filename"); + for(const Containers::StringView name: filenames) { + if(name != filename) continue; + + /* Load the file */ + Containers::Optional> data = Path::read(Path::join(Path::split(_overrideGroup->conf.filename()).first(), filename)); + if(!data) { + Error{} << "Utility::Resource::get(): cannot open file" << name << "from overridden group"; + break; + } + + /* Save the file for later use and return. Use a filename from the + compiled-in resources which is guaranteed to be global to avoid + allocating a new string */ + it = _overrideGroup->data.emplace(Implementation::resourceFilenameAt(_group->positions, _group->filenames, i), *std::move(data)).first; + return Containers::ArrayView{it->second}; + } + + /* Then [file] groups */ const std::vector files = _overrideGroup->conf.groups("file"); for(const ConfigurationGroup* const file: files) { const Containers::StringView name = file->hasValue("alias") ? file->value("alias") : file->value("filename"); diff --git a/src/Corrade/Utility/Test/CMakeLists.txt b/src/Corrade/Utility/Test/CMakeLists.txt index c487d2897..3b3feb33a 100644 --- a/src/Corrade/Utility/Test/CMakeLists.txt +++ b/src/Corrade/Utility/Test/CMakeLists.txt @@ -395,9 +395,11 @@ corrade_add_test(UtilityResourceCompileTest ResourceTestFiles/resources-alignment-larger-than-data-size.conf ResourceTestFiles/resources-empty-alias.conf ResourceTestFiles/resources-empty-filename.conf + ResourceTestFiles/resources-empty-file-filename.conf ResourceTestFiles/resources-empty-group.conf ResourceTestFiles/resources-no-group.conf ResourceTestFiles/resources-nonexistent.conf + ResourceTestFiles/resources-file-nonexistent.conf ResourceTestFiles/resources-nothing.conf ResourceTestFiles/resources-npot-align.conf ResourceTestFiles/resources-npot-global-align.conf diff --git a/src/Corrade/Utility/Test/ResourceCompileTest.cpp b/src/Corrade/Utility/Test/ResourceCompileTest.cpp index e299b1a77..8067cda47 100644 --- a/src/Corrade/Utility/Test/ResourceCompileTest.cpp +++ b/src/Corrade/Utility/Test/ResourceCompileTest.cpp @@ -71,13 +71,18 @@ const struct { } CompileFromInvalidData[]{ {"nonexistent resource file", "/nonexistent.conf", "file /nonexistent.conf does not exist"}, - {"nonexistent file", "resources-nonexistent.conf", + {"nonexistent filename", "resources-nonexistent.conf", + /* There's an error message from Path::read() before */ + "\n Error: cannot open file /nonexistent.dat of file 1 in group name\n"}, + {"nonexistent [file] filename", "resources-file-nonexistent.conf", /* There's an error message from Path::read() before */ "\n Error: cannot open file /nonexistent.dat of file 1 in group name\n"}, /* Empty group= option is allowed, tested in compileFromEmptyGroup() */ {"empty group", "resources-no-group.conf", "group name is not specified"}, {"empty filename", "resources-empty-filename.conf", + "filename 2 in group name is empty"}, + {"empty [file] filename", "resources-empty-file-filename.conf", "filename or alias of file 1 in group name is empty"}, {"empty alias", "resources-empty-alias.conf", "filename or alias of file 1 in group name is empty"}, diff --git a/src/Corrade/Utility/Test/ResourceTest.cpp b/src/Corrade/Utility/Test/ResourceTest.cpp index 03e3523e5..084f325a0 100644 --- a/src/Corrade/Utility/Test/ResourceTest.cpp +++ b/src/Corrade/Utility/Test/ResourceTest.cpp @@ -528,6 +528,8 @@ void ResourceTest::overrideGroup() { /* Two subsequent calls should point to the same location (the file doesn't get read again) */ CORRADE_VERIFY(rs.getString("predisposition.bin").data() == predisposition.data()); + + // TODO test both filename= and [file] } void ResourceTest::overrideGroupNonexistent() { diff --git a/src/Corrade/Utility/Test/ResourceTestFiles/resources-empty-file-filename.conf b/src/Corrade/Utility/Test/ResourceTestFiles/resources-empty-file-filename.conf new file mode 100644 index 000000000..ae2028fe9 --- /dev/null +++ b/src/Corrade/Utility/Test/ResourceTestFiles/resources-empty-file-filename.conf @@ -0,0 +1,4 @@ +group=name + +[file] +filename= diff --git a/src/Corrade/Utility/Test/ResourceTestFiles/resources-empty-filename.conf b/src/Corrade/Utility/Test/ResourceTestFiles/resources-empty-filename.conf index ae2028fe9..76c7e7cae 100644 --- a/src/Corrade/Utility/Test/ResourceTestFiles/resources-empty-filename.conf +++ b/src/Corrade/Utility/Test/ResourceTestFiles/resources-empty-filename.conf @@ -1,4 +1,4 @@ group=name -[file] +filename=empty.bin filename= diff --git a/src/Corrade/Utility/Test/ResourceTestFiles/resources-file-nonexistent.conf b/src/Corrade/Utility/Test/ResourceTestFiles/resources-file-nonexistent.conf new file mode 100644 index 000000000..3ebcacaac --- /dev/null +++ b/src/Corrade/Utility/Test/ResourceTestFiles/resources-file-nonexistent.conf @@ -0,0 +1,5 @@ +group=name + +[file] +filename=/nonexistent.dat +alias=but-it-exists.dat diff --git a/src/Corrade/Utility/Test/ResourceTestFiles/resources-nonexistent.conf b/src/Corrade/Utility/Test/ResourceTestFiles/resources-nonexistent.conf index 3ebcacaac..f8bfb47f7 100644 --- a/src/Corrade/Utility/Test/ResourceTestFiles/resources-nonexistent.conf +++ b/src/Corrade/Utility/Test/ResourceTestFiles/resources-nonexistent.conf @@ -1,5 +1,2 @@ group=name - -[file] filename=/nonexistent.dat -alias=but-it-exists.dat diff --git a/src/Corrade/Utility/Test/ResourceTestFiles/resources-overridden.conf b/src/Corrade/Utility/Test/ResourceTestFiles/resources-overridden.conf index 072693bad..76f323ce1 100644 --- a/src/Corrade/Utility/Test/ResourceTestFiles/resources-overridden.conf +++ b/src/Corrade/Utility/Test/ResourceTestFiles/resources-overridden.conf @@ -1,8 +1,7 @@ group=test +filename=consequence2.txt + [file] filename=../ResourceTestFiles/predisposition2.txt alias=predisposition.bin - -[file] -filename=consequence2.txt diff --git a/src/Corrade/Utility/Test/ResourceTestFiles/resources-spaces.conf b/src/Corrade/Utility/Test/ResourceTestFiles/resources-spaces.conf index e0c4ac481..24881393e 100644 --- a/src/Corrade/Utility/Test/ResourceTestFiles/resources-spaces.conf +++ b/src/Corrade/Utility/Test/ResourceTestFiles/resources-spaces.conf @@ -1,12 +1,10 @@ group=spaces -[file] # In this case the spaces should be trimmed from the beginning and the end, but # not from inside the quotes, DO NOT CLEAN UP THE TRAILING SPACE # vvv----------------------vvv filename= "name with spaces.txt" -[file] # Here's a leading and trailing space, DO NOT CLEAN UP # v------------------v filename= predisposition.bin diff --git a/src/Corrade/Utility/Test/ResourceTestFiles/resources.conf b/src/Corrade/Utility/Test/ResourceTestFiles/resources.conf index 335bf68a1..4ac1bd8bc 100644 --- a/src/Corrade/Utility/Test/ResourceTestFiles/resources.conf +++ b/src/Corrade/Utility/Test/ResourceTestFiles/resources.conf @@ -4,5 +4,8 @@ group=test filename=../ResourceTestFiles/predisposition.bin alias=predisposition.bin +# Loose filename= options are tested in resources-spaces.conf and +# resources-overriden.conf but not here because this tests that the filenames +# get correctly sorted [file] filename=consequence.bin