From 856da036eb0cbd041469ef8a28f0cfb2f1743c3b Mon Sep 17 00:00:00 2001 From: Vojtech Bubnik Date: Mon, 28 Jun 2021 17:26:24 +0200 Subject: [PATCH] Fixed loading of system presets with incompatible system profile keys before the "reconfigure" dialog is shown. Replaced boost::filesystem::copy_file() with Slic3r::copy_file() in config snapshot loading code. --- src/libslic3r/Config.cpp | 3 +- src/libslic3r/Config.hpp | 7 +++++ src/libslic3r/PresetBundle.cpp | 44 +++++++++++++++++------------- src/libslic3r/PresetBundle.hpp | 8 ++++-- src/slic3r/Config/Snapshot.cpp | 8 ++++-- src/slic3r/GUI/ConfigWizard.cpp | 7 +++-- src/slic3r/GUI/GUI_App.cpp | 8 +++++- src/slic3r/GUI/MainFrame.cpp | 4 ++- src/slic3r/Utils/PresetUpdater.cpp | 10 +++++-- 9 files changed, 67 insertions(+), 32 deletions(-) diff --git a/src/libslic3r/Config.cpp b/src/libslic3r/Config.cpp index b7facbe0e..ae154bf27 100644 --- a/src/libslic3r/Config.cpp +++ b/src/libslic3r/Config.cpp @@ -553,7 +553,8 @@ bool ConfigBase::set_deserialize_raw(const t_config_option_key &opt_key_src, con opt->set(optdef->default_value.get()); - if (substitutions_ctxt.rule == ForwardCompatibilitySubstitutionRule::Enable) { + if (substitutions_ctxt.rule == ForwardCompatibilitySubstitutionRule::Enable || + substitutions_ctxt.rule == ForwardCompatibilitySubstitutionRule::EnableSystemSilent) { // Log the substitution. ConfigSubstitution config_substitution; config_substitution.opt_def = optdef; diff --git a/src/libslic3r/Config.hpp b/src/libslic3r/Config.hpp index 8690f2eb5..ab3caebb8 100644 --- a/src/libslic3r/Config.hpp +++ b/src/libslic3r/Config.hpp @@ -166,9 +166,16 @@ enum PrinterTechnology : unsigned char enum ForwardCompatibilitySubstitutionRule { + // Disable susbtitution, throw exception if an option value is not recognized. Disable, + // Enable substitution of an unknown option value with default. Log the substitution. Enable, + // Enable substitution of an unknown option value with default. Don't log the substitution. EnableSilent, + // Enable substitution of an unknown option value with default. Log substitutions in user profiles, don't log substitutions in system profiles. + EnableSystemSilent, + // Enable silent substitution of an unknown option value with default when loading user profiles. Throw on an unknown option value in a system profile. + EnableSilentDisableSystem, }; class ConfigOption; diff --git a/src/libslic3r/PresetBundle.cpp b/src/libslic3r/PresetBundle.cpp index 9f089ea1d..53743411c 100644 --- a/src/libslic3r/PresetBundle.cpp +++ b/src/libslic3r/PresetBundle.cpp @@ -191,7 +191,9 @@ void PresetBundle::setup_directories() PresetsConfigSubstitutions PresetBundle::load_presets(AppConfig &config, ForwardCompatibilitySubstitutionRule substitution_rule, const std::string &preferred_model_id) { // First load the vendor specific system presets. - std::string errors_cummulative = this->load_system_presets(); + PresetsConfigSubstitutions substitutions; + std::string errors_cummulative; + std::tie(substitutions, errors_cummulative) = this->load_system_presets(substitution_rule); const std::string dir_user_presets = data_dir() #ifdef SLIC3R_PROFILE_USE_PRESETS_SUBDIR @@ -202,7 +204,6 @@ PresetsConfigSubstitutions PresetBundle::load_presets(AppConfig &config, Forward #endif ; - PresetsConfigSubstitutions substitutions; try { this->prints.load_presets(dir_user_presets, "print", substitutions, substitution_rule); } catch (const std::runtime_error &err) { @@ -245,12 +246,20 @@ PresetsConfigSubstitutions PresetBundle::load_presets(AppConfig &config, Forward // Load system presets into this PresetBundle. // For each vendor, there will be a single PresetBundle loaded. -std::string PresetBundle::load_system_presets() +std::pair PresetBundle::load_system_presets(ForwardCompatibilitySubstitutionRule compatibility_rule) { + if (compatibility_rule == ForwardCompatibilitySubstitutionRule::EnableSystemSilent) + // Loading system presets, don't log substitutions. + compatibility_rule = ForwardCompatibilitySubstitutionRule::EnableSilent; + else if (compatibility_rule == ForwardCompatibilitySubstitutionRule::EnableSilentDisableSystem) + // Loading system presets, throw on unknown option value. + compatibility_rule = ForwardCompatibilitySubstitutionRule::Disable; + // Here the vendor specific read only Config Bundles are stored. - boost::filesystem::path dir = (boost::filesystem::path(data_dir()) / "vendor").make_preferred(); - std::string errors_cummulative; - bool first = true; + boost::filesystem::path dir = (boost::filesystem::path(data_dir()) / "vendor").make_preferred(); + PresetsConfigSubstitutions substitutions; + std::string errors_cummulative; + bool first = true; for (auto &dir_entry : boost::filesystem::directory_iterator(dir)) if (Slic3r::is_ini_file(dir_entry)) { std::string name = dir_entry.path().filename().string(); @@ -260,13 +269,13 @@ std::string PresetBundle::load_system_presets() // Load the config bundle, flatten it. if (first) { // Reset this PresetBundle and load the first vendor config. - this->load_configbundle(dir_entry.path().string(), PresetBundle::LoadSystem); + append(substitutions, this->load_configbundle(dir_entry.path().string(), PresetBundle::LoadSystem, compatibility_rule).first); first = false; } else { // Load the other vendor configs, merge them with this PresetBundle. // Report duplicate profiles. PresetBundle other; - other.load_configbundle(dir_entry.path().string(), PresetBundle::LoadSystem); + append(substitutions, other.load_configbundle(dir_entry.path().string(), PresetBundle::LoadSystem, compatibility_rule).first); std::vector duplicates = this->merge_presets(std::move(other)); if (! duplicates.empty()) { errors_cummulative += "Vendor configuration file " + name + " contains the following presets with names used by other vendors: "; @@ -288,7 +297,7 @@ std::string PresetBundle::load_system_presets() } this->update_system_maps(); - return errors_cummulative; + return std::make_pair(std::move(substitutions), errors_cummulative); } // Merge one vendor's presets with the other vendor's presets, report duplicates. @@ -739,7 +748,7 @@ ConfigSubstitutions PresetBundle::load_config_file(const std::string &path, Forw return config_substitutions; } case CONFIG_FILE_TYPE_CONFIG_BUNDLE: - return load_config_file_config_bundle(path, tree); + return load_config_file_config_bundle(path, tree, compatibility_rule); } // This shall never happen. Suppres compiler warnings. @@ -916,13 +925,14 @@ void PresetBundle::load_config_file_config(const std::string &name_or_path, bool } // Load the active configuration of a config bundle from a boost property_tree. This is a private method called from load_config_file. -ConfigSubstitutions PresetBundle::load_config_file_config_bundle(const std::string &path, const boost::property_tree::ptree &tree) +ConfigSubstitutions PresetBundle::load_config_file_config_bundle( + const std::string &path, const boost::property_tree::ptree &tree, ForwardCompatibilitySubstitutionRule compatibility_rule) { // 1) Load the config bundle into a temp data. PresetBundle tmp_bundle; // Load the config bundle, but don't save the loaded presets to user profile directory, as only the presets marked as active in the loaded preset bundle // will be loaded into the master PresetBundle and activated. - auto [presets_substitutions, presets_imported] = tmp_bundle.load_configbundle(path, {}); + auto [presets_substitutions, presets_imported] = tmp_bundle.load_configbundle(path, {}, compatibility_rule); UNUSED(presets_imported); std::string bundle_name = std::string(" - ") + boost::filesystem::path(path).filename().string(); @@ -1135,15 +1145,11 @@ static void flatten_configbundle_hierarchy(boost::property_tree::ptree &tree, co // Load a config bundle file, into presets and store the loaded presets into separate files // of the local configuration directory. -std::pair PresetBundle::load_configbundle(const std::string &path, LoadConfigBundleAttributes flags) +std::pair PresetBundle::load_configbundle( + const std::string &path, LoadConfigBundleAttributes flags, ForwardCompatibilitySubstitutionRule compatibility_rule) { // Enable substitutions for user config bundle, throw an exception when loading a system profile. - ConfigSubstitutionContext substitution_context { - flags.has(LoadConfigBundleAttribute::LoadSystem) ? - ForwardCompatibilitySubstitutionRule::Disable : - ForwardCompatibilitySubstitutionRule::Enable - }; - + ConfigSubstitutionContext substitution_context { compatibility_rule }; PresetsConfigSubstitutions substitutions; if (flags.has(LoadConfigBundleAttribute::ResetUserProfile) || flags.has(LoadConfigBundleAttribute::LoadSystem)) diff --git a/src/libslic3r/PresetBundle.hpp b/src/libslic3r/PresetBundle.hpp index 9f75ba6c2..3c7349668 100644 --- a/src/libslic3r/PresetBundle.hpp +++ b/src/libslic3r/PresetBundle.hpp @@ -102,7 +102,8 @@ public: using LoadConfigBundleAttributes = enum_bitmask; // Load the config bundle based on the flags. // Don't do any config substitutions when loading a system profile, perform and report substitutions otherwise. - std::pair load_configbundle(const std::string &path, LoadConfigBundleAttributes flags); + std::pair load_configbundle( + const std::string &path, LoadConfigBundleAttributes flags, ForwardCompatibilitySubstitutionRule compatibility_rule); // Export a config bundle file containing all the presets and the names of the active presets. void export_configbundle(const std::string &path, bool export_system_settings = false, bool export_physical_printers = false); @@ -139,7 +140,7 @@ public: static const char *PRUSA_BUNDLE; private: - std::string load_system_presets(); + std::pair load_system_presets(ForwardCompatibilitySubstitutionRule compatibility_rule); // Merge one vendor's presets with the other vendor's presets, report duplicates. std::vector merge_presets(PresetBundle &&other); // Update renamed_from and alias maps of system profiles. @@ -158,7 +159,8 @@ private: // and the external config is just referenced, not stored into user profile directory. // If it is not an external config, then the config will be stored into the user profile directory. void load_config_file_config(const std::string &name_or_path, bool is_external, DynamicPrintConfig &&config); - ConfigSubstitutions load_config_file_config_bundle(const std::string &path, const boost::property_tree::ptree &tree); + ConfigSubstitutions load_config_file_config_bundle( + const std::string &path, const boost::property_tree::ptree &tree, ForwardCompatibilitySubstitutionRule compatibility_rule); DynamicPrintConfig full_fff_config() const; DynamicPrintConfig full_sla_config() const; diff --git a/src/slic3r/Config/Snapshot.cpp b/src/slic3r/Config/Snapshot.cpp index 56722173b..40aade783 100644 --- a/src/slic3r/Config/Snapshot.cpp +++ b/src/slic3r/Config/Snapshot.cpp @@ -10,6 +10,7 @@ #include #include "libslic3r/PresetBundle.hpp" +#include "libslic3r/format.hpp" #include "libslic3r/libslic3r.h" #include "libslic3r/Time.hpp" #include "libslic3r/Config.hpp" @@ -358,11 +359,12 @@ static void copy_config_dir_single_level(const boost::filesystem::path &path_src { if (! boost::filesystem::is_directory(path_dst) && ! boost::filesystem::create_directory(path_dst)) - throw Slic3r::RuntimeError(std::string("Slic3r was unable to create a directory at ") + path_dst.string()); + throw Slic3r::RuntimeError(std::string("PrusaSlicer was unable to create a directory at ") + path_dst.string()); for (auto &dir_entry : boost::filesystem::directory_iterator(path_src)) if (Slic3r::is_ini_file(dir_entry)) - boost::filesystem::copy_file(dir_entry.path(), path_dst / dir_entry.path().filename(), boost::filesystem::copy_option::overwrite_if_exists); + if (std::string error_message; copy_file(dir_entry.path().string(), (path_dst / dir_entry.path().filename()).string(), error_message, false) != SUCCESS) + throw Slic3r::RuntimeError(format("Failed copying \"%1%\" to \"%2%\": %3%", path_src.string(), path_dst.string(), error_message)); } static void delete_existing_ini_files(const boost::filesystem::path &path) @@ -413,7 +415,7 @@ const Snapshot& SnapshotDB::take_snapshot(const AppConfig &app_config, Snapshot: ++ it; // Read the active config bundle, parse the config version. PresetBundle bundle; - bundle.load_configbundle((data_dir / "vendor" / (cfg.name + ".ini")).string(), PresetBundle::LoadConfigBundleAttribute::LoadVendorOnly); + bundle.load_configbundle((data_dir / "vendor" / (cfg.name + ".ini")).string(), PresetBundle::LoadConfigBundleAttribute::LoadVendorOnly, ForwardCompatibilitySubstitutionRule::EnableSilent); for (const auto &vp : bundle.vendors) if (vp.second.id == cfg.name) cfg.version.config_version = vp.second.config_version; diff --git a/src/slic3r/GUI/ConfigWizard.cpp b/src/slic3r/GUI/ConfigWizard.cpp index 8bf8cd3d7..e873d9b07 100644 --- a/src/slic3r/GUI/ConfigWizard.cpp +++ b/src/slic3r/GUI/ConfigWizard.cpp @@ -65,7 +65,9 @@ bool Bundle::load(fs::path source_path, bool ais_in_resources, bool ais_prusa_bu this->is_prusa_bundle = ais_prusa_bundle; std::string path_string = source_path.string(); - auto [config_substitutions, presets_loaded] = preset_bundle->load_configbundle(path_string, PresetBundle::LoadConfigBundleAttribute::LoadSystem); + // Throw when parsing invalid configuration. Only valid configuration is supposed to be provided over the air. + auto [config_substitutions, presets_loaded] = preset_bundle->load_configbundle( + path_string, PresetBundle::LoadConfigBundleAttribute::LoadSystem, ForwardCompatibilitySubstitutionRule::Disable); UNUSED(config_substitutions); // No substitutions shall be reported when loading a system config bundle, no substitutions are allowed. assert(config_substitutions.empty()); @@ -2590,7 +2592,8 @@ void ConfigWizard::priv::apply_config(AppConfig *app_config, PresetBundle *prese // Reloading the configs after some modifications were done to PrusaSlicer.ini. // Just perform the substitutions silently, as the substitutions were already presented to the user on application start-up // and the Wizard shall not create any new values that would require substitution. - PresetsConfigSubstitutions substitutions = preset_bundle->load_presets(*app_config, ForwardCompatibilitySubstitutionRule::EnableSilent, preferred_model); + // Throw on substitutions in system profiles, as the system profiles provided over the air should be compatible with this PrusaSlicer version. + preset_bundle->load_presets(*app_config, ForwardCompatibilitySubstitutionRule::EnableSilentDisableSystem, preferred_model); if (page_custom->custom_wanted()) { page_firmware->apply_custom_config(*custom_config); diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index 3205bddca..6eb0547c3 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/src/slic3r/GUI/GUI_App.cpp @@ -912,7 +912,10 @@ bool GUI_App::on_init_inner() // Suppress the '- default -' presets. preset_bundle->set_default_suppressed(app_config->get("no_defaults") == "1"); try { - init_params->preset_substitutions = preset_bundle->load_presets(*app_config, ForwardCompatibilitySubstitutionRule::Enable); + // Enable all substitutions (in both user and system profiles), but log the substitutions in user profiles only. + // If there are substitutions in system profiles, then a "reconfigure" event shall be triggered, which will force + // installation of a compatible system preset, thus nullifying the system preset substitutions. + init_params->preset_substitutions = preset_bundle->load_presets(*app_config, ForwardCompatibilitySubstitutionRule::EnableSystemSilent); } catch (const std::exception &ex) { show_error(nullptr, ex.what()); } @@ -1874,6 +1877,9 @@ void GUI_App::add_config_menu(wxMenuBar *menu) Config::SnapshotDB::singleton().take_snapshot(*app_config, Config::Snapshot::SNAPSHOT_BEFORE_ROLLBACK); try { app_config->set("on_snapshot", Config::SnapshotDB::singleton().restore_snapshot(dlg.snapshot_to_activate(), *app_config).id); + // Enable substitutions, log both user and system substitutions. There should not be any substitutions performed when loading system + // presets because compatibility of profiles shall be verified using the min_slic3r_version keys in config index, but users + // are known to be creative and mess with the config files in various ways. if (PresetsConfigSubstitutions all_substitutions = preset_bundle->load_presets(*app_config, ForwardCompatibilitySubstitutionRule::Enable); ! all_substitutions.empty()) show_substitutions_info(all_substitutions); diff --git a/src/slic3r/GUI/MainFrame.cpp b/src/slic3r/GUI/MainFrame.cpp index 4f53d3f01..93641adff 100644 --- a/src/slic3r/GUI/MainFrame.cpp +++ b/src/slic3r/GUI/MainFrame.cpp @@ -1809,7 +1809,9 @@ void MainFrame::load_configbundle(wxString file/* = wxEmptyString, const bool re size_t presets_imported = 0; PresetsConfigSubstitutions config_substitutions; try { - std::tie(config_substitutions, presets_imported) = wxGetApp().preset_bundle->load_configbundle(file.ToUTF8().data(), PresetBundle::LoadConfigBundleAttribute::SaveImported); + // Report all substitutions. + std::tie(config_substitutions, presets_imported) = wxGetApp().preset_bundle->load_configbundle( + file.ToUTF8().data(), PresetBundle::LoadConfigBundleAttribute::SaveImported, ForwardCompatibilitySubstitutionRule::Enable); } catch (const std::exception &ex) { show_error(this, ex.what()); return; diff --git a/src/slic3r/Utils/PresetUpdater.cpp b/src/slic3r/Utils/PresetUpdater.cpp index 078c2fe20..7fcf1ed68 100644 --- a/src/slic3r/Utils/PresetUpdater.cpp +++ b/src/slic3r/Utils/PresetUpdater.cpp @@ -65,6 +65,10 @@ void copy_file_fix(const fs::path &source, const fs::path &target) _L("Copying of file %1% to %2% failed: %3%"), source, target, error_message)); } + // Permissions should be copied from the source file by copy_file(). We are not sure about the source + // permissions, let's rewrite them with 644. + static constexpr const auto perms = fs::owner_read | fs::owner_write | fs::group_read | fs::others_read; + fs::permissions(target, perms); } struct Update @@ -611,7 +615,8 @@ void PresetUpdater::priv::perform_updates(Updates &&updates, bool snapshot) cons update.install(); PresetBundle bundle; - bundle.load_configbundle(update.source.string(), PresetBundle::LoadConfigBundleAttribute::LoadSystem); + // Throw when parsing invalid configuration. Only valid configuration is supposed to be provided over the air. + bundle.load_configbundle(update.source.string(), PresetBundle::LoadConfigBundleAttribute::LoadSystem, ForwardCompatibilitySubstitutionRule::Disable); BOOST_LOG_TRIVIAL(info) << format("Deleting %1% conflicting presets", bundle.prints.size() + bundle.filaments.size() + bundle.printers.size()); @@ -715,7 +720,8 @@ static void reload_configs_update_gui() auto* app_config = GUI::wxGetApp().app_config; // System profiles should not trigger any substitutions, user profiles may trigger substitutions, but these substitutions // were already presented to the user on application start up. Just do substitutions now and keep quiet about it. - GUI::wxGetApp().preset_bundle->load_presets(*app_config, ForwardCompatibilitySubstitutionRule::EnableSilent); + // However throw on substitutions in system profiles, those shall never happen with system profiles installed over the air. + GUI::wxGetApp().preset_bundle->load_presets(*app_config, ForwardCompatibilitySubstitutionRule::EnableSilentDisableSystem); GUI::wxGetApp().load_current_presets(); GUI::wxGetApp().plater()->set_bed_shape(); }