diff --git a/.github/workflows/mac.yml b/.github/workflows/mac.yml index 1ae8ceb2..0ca8a4fb 100644 --- a/.github/workflows/mac.yml +++ b/.github/workflows/mac.yml @@ -39,6 +39,11 @@ jobs: - uses: actions/checkout@v3 with: fetch-depth: 0 + # + # Tried to install MacPorts from the bt script, but get errors running its configure script, so trying from GitHub + # actions. + # + - uses: melusina-org/setup-macports@v1 # # The `brew doctor` command just checks that Homebrew (https://brew.sh/) is installed OK (expected output is "Your @@ -53,7 +58,7 @@ jobs: run: | echo "Output from brew doctor: $(brew doctor)" echo "Output from xcode-select -p: $(xcode-select -p)" - brew install python@3.11 + brew install python@3.12 echo "Python3 ($(which python3)) version" /usr/bin/env python3 --version echo "Running ./bt -v setup all" diff --git a/bt b/bt index e826b502..16528683 100755 --- a/bt +++ b/bt @@ -825,6 +825,33 @@ def installDependencies(): case 'Darwin': log.debug('Mac') # + # We install most of the Mac dependencies via Homebrew (https://brew.sh/) using the `brew` command below. + # However, as at 2023-12-01, Homebrew has stopped supplying a package for Xalan-C. So, we install that using + # MacPorts (https://ports.macports.org/), which provides the `port` command. + # + # Note that MacPorts (port) requires sudo but Homebrew (brew) does not. Perhaps more importantly, they two + # package managers install things to different locations: + # - Homebrew packages are installed under /usr/local/Cellar/ with symlinks in /usr/local/opt/ + # - MacPorts packages are installed under /opt/local + # This means we need to have both directories in the include path when we come to compile. Thankfully, both + # CMake and Meson take care of finding a library automatically once given its name. + # + # Note too that package names vary slightly between HomeBrew and MacPorts. Don't assume you can guess one from + # the other, as it's not always evident. + # + + # + # Installing Homebrew is, in theory, somewhat easier and more self-contained than MacPorts as you just run the + # following: + # /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)" + # In practice, invoking that command from this script is a bit fiddly to get right. For the moment, we simply + # assume Homebrew is already installed (because it is on the GitHub actions). + # + + # + # We install as many of our dependencies as possible with with Homebrew, and do this first, because some of + # these packages will also be needed for the installation of MacPorts to work. + # # We could make this list shorter if we wanted as, eg, installing Xalan-C will cause Xerces-C to be installed # too (as the former depends on the latter). However, I think it's clearer to explicitly list all the direct # dependencies (eg we do make calls directly into Xerces). @@ -840,22 +867,23 @@ def installDependencies(): # diagnosing certain build problems (eg to see what changes certain parts of the build have made to the build # directory tree) when the build is running as a GitHub action. # - installList = ['boost', - 'cmake', - 'coreutils', - 'doxygen', - 'gcc', - 'git', - 'llvm', - 'meson', - 'ninja', - 'pandoc', - 'tree', - 'qt@5', - 'xalan-c', - 'xerces-c'] - for packageToInstall in installList: - log.debug('Installing ' + packageToInstall) + installListBrew = ['llvm', + 'gcc', + 'cmake', + 'coreutils', + 'boost', + 'doxygen', + 'git', + 'meson', + 'ninja', + 'pandoc', + 'tree', + 'qt@5', +# 'xalan-c', +# 'xerces-c' + ] + for packageToInstall in installListBrew: + log.debug('Installing ' + packageToInstall + ' via Homebrew') abortOnRunFail(subprocess.run(['brew', 'install', packageToInstall])) # # By default, even once Qt5 is installed, Meson will not find it @@ -874,6 +902,37 @@ def installDependencies(): # abortOnRunFail(subprocess.run(['brew', 'link', '--force', 'qt5'])) + # + # In theory, we can now install MacPorts. However, I didn't manage to get the following working. The + # configure script complains about the lack of /usr/local/.//mkspecs/macx-clang. So, for now, we comment this + # bit out and install MacPorts for GitHub actions via the mac.yml script. + # + # This is a source install - per instructions at https://guide.macports.org/#installing.macports.source. In + # principle, we could install a precompiled binary, but it's a bit painful to do programatically as even to + # download the right package you have to know not just the the Darwin version of MacOS you are running, but + # also its "release name" (aka "friendly name"). See + # https://apple.stackexchange.com/questions/333452/how-can-i-find-the-friendly-name-of-the-operating-system-from-the-shell-term + # for various ways to do this if we had to, though we might just as easily copy the info + # from https://en.wikipedia.org/wiki/MacOS#Timeline_of_releases + # +## log.debug('Installing MacPorts') +## abortOnRunFail(subprocess.run(['curl', '-O', 'https://distfiles.macports.org/MacPorts/MacPorts-2.8.1.tar.bz2'])) +## abortOnRunFail(subprocess.run(['tar', 'xf', 'MacPorts-2.8.1.tar.bz2'])) +## abortOnRunFail(subprocess.run(['cd', 'MacPorts-2.8.1/'])) +## abortOnRunFail(subprocess.run(['./configure'])) +## abortOnRunFail(subprocess.run(['make'])) +## abortOnRunFail(subprocess.run(['sudo', 'make', 'install'])) +## abortOnRunFail(subprocess.run(['export', 'PATH=/opt/local/bin:/opt/local/sbin:$PATH'])) + + # + # Now install Xalan-C via MacPorts + # + installListPort = ['xalanc', + 'xercesc3'] + for packageToInstall in installListPort: + log.debug('Installing ' + packageToInstall + ' via MacPorts') + abortOnRunFail(subprocess.run(['sudo', 'port', 'install', packageToInstall])) + # # dmgbuild is a Python package that provides a command line tool to create macOS disk images (aka .dmg # files) -- see https://dmgbuild.readthedocs.io/en/latest/ diff --git a/src/database/ObjectStore.cpp b/src/database/ObjectStore.cpp index d860092f..ef94b964 100644 --- a/src/database/ObjectStore.cpp +++ b/src/database/ObjectStore.cpp @@ -583,9 +583,11 @@ class ObjectStore::impl { /** * Constructor */ - impl(TypeLookup const & typeLookup, + impl(char const * const className, + TypeLookup const & typeLookup, TableDefinition const & primaryTable, - JunctionTableDefinitions const & junctionTables) : m_state{ObjectStore::State::NotYetInitialised}, + JunctionTableDefinitions const & junctionTables) : m_className{className}, + m_state{ObjectStore::State::NotYetInitialised}, typeLookup{typeLookup}, primaryTable{primaryTable}, junctionTables{junctionTables}, @@ -768,44 +770,57 @@ class ObjectStore::impl { /// propertyValue.typeName() << "(" << propertyType << ") in column" << fieldDefn.columnName << /// ". This is a known ugliness that we intend to fix one day."; } else { - // It's not a known exception, so it's a coding error. However, we may still be able to recover. - // If we are expecting a boolean and we get a string holding "true" or "false" etc, then we know what to - // do. + // + // It's not a known exception, so it's a coding error. However, we can recover in the following cases: + // - If we are expecting a boolean and we get a string holding "true" or "false" etc, then we know + // what to do. + // - If we are expecting an int and we get a double then we can just ignore the non-integer part of + // the number. + // - If we are expecting an double and we got an int then we can just upcast it. + // bool recovered = false; + QVariant readPropertyValue = propertyValue; if (propertyType == QMetaType::QString && fieldDefn.fieldType == ObjectStore::FieldType::Bool) { // We'll take any reasonable string representation of true/false. For the moment, at least, I'm not // worrying about optional fields here. I think it's pretty rare, if ever, that we'd want an optional // boolean. QString propertyAsLcString = propertyValue.toString().trimmed().toLower(); - bool interpretedValue = false; if (propertyAsLcString == "true" || propertyAsLcString == "t" || propertyAsLcString == "1") { recovered = true; - interpretedValue = true; + propertyValue = QVariant(true); } else if (propertyAsLcString == "false" || propertyAsLcString == "f" || propertyAsLcString == "0") { recovered = true; - interpretedValue = false; - } - if (recovered) { - qWarning() << - Q_FUNC_INFO << "Recovered from unexpected type #" << propertyType << "=" << - propertyValue.typeName() << "in QVariant for property" << fieldDefn.propertyName << - ", field type" << fieldDefn.fieldType << ", value" << propertyValue << ", table" << - primaryTable.tableName << ", column" << fieldDefn.columnName << ". Interpreted value as" << - interpretedValue; - // Now we overwrite the supplied variant with what we worked out it should be, so processing can - // continue. - propertyValue = QVariant(interpretedValue); + propertyValue = QVariant(false); } + } else if (propertyType == QMetaType::Double && fieldDefn.fieldType == ObjectStore::FieldType::Int) { + propertyValue = QVariant(propertyValue.toInt(&recovered)); + } else if (propertyType == QMetaType::Int && fieldDefn.fieldType == ObjectStore::FieldType::Double) { + propertyValue = QVariant(propertyValue.toDouble(&recovered)); } - if (!recovered) { + if (recovered) { + qWarning() << + Q_FUNC_INFO << "Recovered from unexpected type #" << propertyType << "=" << + readPropertyValue.typeName() << "in QVariant for property" << fieldDefn.propertyName << + ", field type" << fieldDefn.fieldType << ", value" << readPropertyValue << ", table" << + primaryTable.tableName << ", column" << fieldDefn.columnName << ". Interpreted value as" << + propertyValue; + } else { + // + // Even in the case where we do not have a reasonable way to interpret the data in this column, we + // should probably NOT terminate the program. We are still discovering lots of cases where folks + // using the software have old Brewtarget databases with quirky values in some columns. It's better + // from a user point of view that the software carries on working even if some (hopefully) obscure + // field could not be read from the DB. + // qCritical() << Q_FUNC_INFO << "Unexpected type #" << propertyType << "=" << propertyValue.typeName() << "in QVariant for property" << fieldDefn.propertyName << ", field type" << fieldDefn.fieldType << ", value" << propertyValue << ", table" << primaryTable.tableName << ", column" << fieldDefn.columnName; - qCritical().noquote() << Q_FUNC_INFO << "Call stack is:" << Logging::getStackTrace(); - // Stop here on debug build - Q_ASSERT(false); + // If, during development or debugging, you want to have the program stop when it cannot interpret + // one of the DB fields, then uncomment the following two lines. +/// qCritical().noquote() << Q_FUNC_INFO << "Call stack is:" << Logging::getStackTrace(); +/// Q_ASSERT(false); } } @@ -1232,6 +1247,7 @@ class ObjectStore::impl { return primaryKeyInDb; } + char const * const m_className; ObjectStore::State m_state; TypeLookup const & typeLookup; TableDefinition const & primaryTable; @@ -1259,10 +1275,11 @@ QString ObjectStore::getDisplayName(ObjectStore::FieldType const fieldType) { Q_ASSERT(false); } -ObjectStore::ObjectStore(TypeLookup const & typeLookup, +ObjectStore::ObjectStore(char const * const className, + TypeLookup const & typeLookup, TableDefinition const & primaryTable, JunctionTableDefinitions const & junctionTables) : - pimpl{ std::make_unique(typeLookup, primaryTable, junctionTables) } { + pimpl{ std::make_unique(className, typeLookup, primaryTable, junctionTables) } { qDebug() << Q_FUNC_INFO << "Construct of object store for primary table" << this->pimpl->primaryTable.tableName; // We have seen a circumstance where primaryTable.tableName is null, which shouldn't be possible. This is some // diagnostic to try to find out why. @@ -1839,12 +1856,12 @@ void ObjectStore::updateProperty(QObject const & object, BtStringConst const & p return; } -std::shared_ptr ObjectStore::defaultSoftDelete(int id) { +std::shared_ptr ObjectStore::defaultSoftDelete(int id) { // // We assume on soft-delete that there is nothing to do on related objects - eg if a Mash is soft deleted (ie marked // deleted but remains in the DB) then there isn't actually anything we need to do with its MashSteps. // - qDebug() << Q_FUNC_INFO << "Soft delete item #" << id; + qDebug() << Q_FUNC_INFO << "Soft delete" << this->pimpl->m_className << "#" << id; auto object = this->pimpl->allObjects.value(id); if (this->pimpl->allObjects.contains(id)) { this->pimpl->allObjects.remove(id); @@ -1856,14 +1873,14 @@ std::shared_ptr ObjectStore::defaultSoftDelete(int id) { return object; } -std::shared_ptr ObjectStore::defaultHardDelete(int id) { +std::shared_ptr ObjectStore::defaultHardDelete(int id) { // // We assume on hard-delete that the subclass ObjectStore (specifically ObjectStoreTyped) will override this member // function to interact with the object to delete any "owned" objects. It is better to have the rules for that in // the object model than here in the object store as they can be subtle, and it would be cumbersome to model them // generically. // - qDebug() << Q_FUNC_INFO << "Hard delete item #" << id; + qDebug() << Q_FUNC_INFO << "Hard delete" << this->pimpl->m_className << "#" << id; auto object = this->pimpl->allObjects.value(id); QSqlDatabase connection = this->pimpl->database->sqlDatabase(); DbTransaction dbTransaction{*this->pimpl->database, connection}; @@ -1993,10 +2010,11 @@ QList ObjectStore::findAllMatching(std::function con QVector ObjectStore::idsOfAllMatching( std::function const & matchFunction ) const { + qDebug() << Q_FUNC_INFO << this->pimpl->m_className; // It would be nice to use C++20 ranges here, but I couldn't find a way to use them with QHash in such a way that the // keys of the hash would be accessible in the range. So, for now, we do it the old way. QVector results; - for (auto hashEntry = this->pimpl->allObjects.cbegin(); hashEntry != this->pimpl->allObjects.cbegin(); ++hashEntry) { + for (auto hashEntry = this->pimpl->allObjects.cbegin(); hashEntry != this->pimpl->allObjects.cend(); ++hashEntry) { if (matchFunction(hashEntry.value().get())) { results.append(hashEntry.key()); } diff --git a/src/database/ObjectStore.h b/src/database/ObjectStore.h index b40db5be..643e8e2b 100644 --- a/src/database/ObjectStore.h +++ b/src/database/ObjectStore.h @@ -205,12 +205,14 @@ class ObjectStore : public QObject { /** * \brief Constructor sets up mappings but does not read in data from DB * + * \param className Set by \c ObjectStoreTyped * \param typeLookup The \c TypeLookup object that, amongst other things allows us to tell whether Qt properties on * this object type are "optional" (ie wrapped in \c std::optional) * \param primaryTable First in the list should be the primary key * \param junctionTables Optional */ - ObjectStore(TypeLookup const & typeLookup, + ObjectStore(char const * const className, + TypeLookup const & typeLookup, TableDefinition const & primaryTable, JunctionTableDefinitions const & junctionTables = JunctionTableDefinitions{}); diff --git a/src/database/ObjectStoreTyped.h b/src/database/ObjectStoreTyped.h index 3bfea505..1ef56241 100644 --- a/src/database/ObjectStoreTyped.h +++ b/src/database/ObjectStoreTyped.h @@ -39,7 +39,7 @@ class ObjectStoreTyped : public ObjectStore { ObjectStoreTyped(TypeLookup const & typeLookup, TableDefinition const & primaryTable, JunctionTableDefinitions const & junctionTables = JunctionTableDefinitions{}) : - ObjectStore(typeLookup, primaryTable, junctionTables) { + ObjectStore(NE::staticMetaObject.className(), typeLookup, primaryTable, junctionTables) { return; } diff --git a/src/model/NamedEntity.h b/src/model/NamedEntity.h index 60697d04..30c537a2 100644 --- a/src/model/NamedEntity.h +++ b/src/model/NamedEntity.h @@ -354,8 +354,8 @@ class NamedEntity : public QObject { * being compared are of the same class (eg we are not comparing a Hop with a Yeast) and that the names match, * so subclasses do not need to repeat these tests. * - * We do not currently anticipate sub-sub-classes of \b NamedEntity but if one ever were created, it should - * call its parent's implementation of this function before doing its own class-specific tests. + * A sub-sub-class of \c NamedEntity (eg \c RecipeAdditionHop) should call its parent's implementation of this + * function before doing its own class-specific tests. * \return \b true if this object is, in all the ways that matter, equal to \b other */ virtual bool isEqualTo(NamedEntity const & other) const = 0; @@ -646,6 +646,12 @@ S & operator<<(S & stream, NamedEntity const * namedEntity) { return stream; } +template +S & operator<<(S & stream, std::shared_ptr namedEntity) { + stream << namedEntity.get(); + return stream; +} + /** * \brief Convenience function for logging, including coping with null pointers * diff --git a/src/model/Recipe.cpp b/src/model/Recipe.cpp index dabd5971..a403aef4 100644 --- a/src/model/Recipe.cpp +++ b/src/model/Recipe.cpp @@ -272,6 +272,7 @@ class Recipe::impl { template void hardDeleteAdditions() { qDebug() << Q_FUNC_INFO; for (int id : this->allMyIds()) { + qDebug() << Q_FUNC_INFO << "Hard deleting" << NE::staticMetaObject.className() << "#" << id; ObjectStoreWrapper::hardDelete(id); } } @@ -2414,8 +2415,11 @@ void Recipe::recalcIBU() { double ibus = 0.0; // Bitterness due to hops... + // + // Note that, normally, we don't want to take a reference to a smart pointer. However, in this context, it's safe + // (because the hop additions aren't going to change while we look at them) and it gets rid of a compiler warning. m_ibus.clear(); - for (auto const hopAddition : this->hopAdditions()) { + for (auto const & hopAddition : this->hopAdditions()) { double tmp = this->ibuFromHopAddition(*hopAddition); m_ibus.append(tmp); ibus += tmp; diff --git a/src/serialization/SerializationRecord.h b/src/serialization/SerializationRecord.h index 7a69df86..39603acd 100644 --- a/src/serialization/SerializationRecord.h +++ b/src/serialization/SerializationRecord.h @@ -25,6 +25,8 @@ /** * \brief Base class for \c XmlRecord and \c JsonRecord * + * TODO: There is more common functionality that could be pulled out into this base class + * * TODO: I think this could be templated on Coding and RecordDefinition */ class SerializationRecord { diff --git a/src/serialization/SerializationRecordDefinition.h b/src/serialization/SerializationRecordDefinition.h index 1546cf36..63d419ca 100644 --- a/src/serialization/SerializationRecordDefinition.h +++ b/src/serialization/SerializationRecordDefinition.h @@ -20,6 +20,7 @@ #include #include +#include #include #include "model/NamedEntity.h" @@ -33,13 +34,19 @@ */ class SerializationRecordDefinition { public: - SerializationRecordDefinition(char const * const recordName, - TypeLookup const * const typeLookup, - char const * const namedEntityClassName, - QVariant (*listUpcaster)(QList> const &)) : + /** + * \param recordName The name of the XML or JSON object for this type of record, eg "fermentables" for a list of + * fermentables in BeerXML. + */ + SerializationRecordDefinition(char const * const recordName, + TypeLookup const * const typeLookup, + char const * const namedEntityClassName, + QString const & localisedEntityName, + QVariant (*listUpcaster)(QList> const &)) : m_recordName {recordName}, m_typeLookup {typeLookup}, m_namedEntityClassName{namedEntityClassName}, + m_localisedEntityName {localisedEntityName}, m_listUpcaster {listUpcaster} { return; } @@ -49,10 +56,18 @@ class SerializationRecordDefinition { TypeLookup const * const m_typeLookup; - // The name of the class of object contained in this type of record, eg "Hop", "Yeast", etc. - // Blank for the root record (which is just a container and doesn't have a NamedEntity). + /** + * The name of the class of object contained in this type of record, eg "Hop", "Yeast", etc. + * Blank for the root record (which is just a container and doesn't have a NamedEntity). + */ BtStringConst const m_namedEntityClassName; + /** + * The localised name of the object, suitable for showing on the screen (eg if we want to tell the user how many Hop + * records were read in, etc). + */ + QString const m_localisedEntityName; + QVariant (*m_listUpcaster)(QList> const &); }; diff --git a/src/serialization/json/BeerJson.cpp b/src/serialization/json/BeerJson.cpp index dff15b34..4087e185 100644 --- a/src/serialization/json/BeerJson.cpp +++ b/src/serialization/json/BeerJson.cpp @@ -223,9 +223,10 @@ namespace { // it's not used) but other compilers do. // template JsonRecordDefinition const BEER_JSON_RECORD_DEFINITION { - "not_used", + "not_used", // JSON record name nullptr, - "not_used", + "not_used", // namedEntityClassName + "not_used", // localisedEntityName nullptr, JsonRecordDefinition::create, std::initializer_list{} @@ -339,8 +340,7 @@ namespace { // As mentioned above, it would be really nice to do this at compile time, but haven't yet found a nice way to do so template<> JsonRecordDefinition const BEER_JSON_RECORD_DEFINITION { std::in_place_type_t{}, - "fermentables", - "Fermentable", + "fermentables", // JSON record name JsonRecordDefinition::create< JsonNamedEntityRecord< Fermentable > >, {BeerJson_FermentableBase, BeerJson_FermentableType_ExclBase} }; @@ -363,8 +363,7 @@ namespace { }; template<> JsonRecordDefinition const BEER_JSON_RECORD_DEFINITION { std::in_place_type_t{}, - "miscellaneous_ingredients", - "Misc", + "miscellaneous_ingredients", // JSON record name JsonRecordDefinition::create< JsonNamedEntityRecord< Misc > >, {BeerJson_MiscellaneousBase, BeerJson_MiscellaneousType_ExclBase} }; @@ -409,8 +408,7 @@ namespace { }; template<> JsonRecordDefinition const BEER_JSON_RECORD_DEFINITION { std::in_place_type_t{}, - "hop_varieties", - "Hop", + "hop_varieties", // JSON record name JsonRecordDefinition::create< JsonNamedEntityRecord< Hop > >, {BeerJson_HopBase, BeerJson_HopType_ExclBase} }; @@ -420,8 +418,7 @@ namespace { ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////// template<> JsonRecordDefinition const BEER_JSON_RECORD_DEFINITION { std::in_place_type_t{}, - "cultures", - "Yeast", + "cultures", // JSON record name JsonRecordDefinition::create< JsonNamedEntityRecord< Yeast > >, { // Type XPath Q_PROPERTY Value Decoder @@ -458,8 +455,7 @@ namespace { ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////// template<> JsonRecordDefinition const BEER_JSON_RECORD_DEFINITION { std::in_place_type_t{}, - "profiles", - "Water", + "profiles", // JSON record name JsonRecordDefinition::create< JsonNamedEntityRecord< Water > >, { // Type XPath Q_PROPERTY Value Decoder @@ -488,8 +484,7 @@ namespace { ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////// template<> JsonRecordDefinition const BEER_JSON_RECORD_DEFINITION