Skip to content

Commit

Permalink
Convert ConfigService paths to absolute on demand
Browse files Browse the repository at this point in the history 
If the key contains the string "director" it is assumed to point
to a directory and if it its value is relatiove then the path
is converted to an absolute path. This reduces the workload on
startup while keeping the caching of important keys
such as the datasearch paths and instrument directories for performance.
Refs #12425
  • Loading branch information
@martyngigg @cailafinn
martyngigg authored and cailafinn committed Jun 4, 2020
1 parent 7a9dbf8 commit d8205f1
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 108 deletions.
11 changes: 1 addition & 10 deletions Framework/Kernel/inc/MantidKernel/ConfigService.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ class MANTID_KERNEL_DLL ConfigServiceImpl final {
void saveConfig(const std::string &filename) const;
/// Searches for a configuration property
std::string getString(const std::string &keyName,
bool use_cache = true) const;
bool pathAbsolute = true) const;
/// Searches for a key in the configuration property
std::vector<std::string> getKeys(const std::string &keyName) const;
/// Returns a list of all full keys in the config
Expand Down Expand Up @@ -255,9 +255,6 @@ class MANTID_KERNEL_DLL ConfigServiceImpl final {

/// Writes out a fresh user properties file
void createUserPropertiesFile() const;
/// Convert any relative paths to absolute ones and store them locally so that
/// if the working directory is altered the paths will not be affected
void convertRelativeToAbsolute();
/// Make a relative path or a list of relative paths into an absolute one.
std::string makeAbsolute(const std::string &dir,
const std::string &key) const;
Expand Down Expand Up @@ -288,12 +285,6 @@ class MANTID_KERNEL_DLL ConfigServiceImpl final {
/// A set of property keys that have been changed
mutable std::set<std::string> m_changed_keys;

/// A map storing string/key pairs where the string denotes a path
/// that could be relative in the user properties file
/// The boolean indicates whether the path needs to exist or not
std::map<std::string, bool> m_ConfigPaths;
/// Local storage for the relative path key/values that have been changed
std::map<std::string, std::string> m_AbsolutePaths;
/// The directory that is considered to be the base directory
std::string m_strBaseDir;
/// The configuration properties in string format
Expand Down
114 changes: 27 additions & 87 deletions Framework/Kernel/src/ConfigService.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,15 @@ namespace { // anonymous namespace for some utility functions
/// static Logger object
Logger g_log("ConfigService");

/// Identifier for a key that will contain a path. This is defined
/// so as to catch both the word directory and directories in a key name.
/// Any keys found containing this string are assumed to contain paths
/// and if their values are relative paths then when fetched they
/// are transformed to absolute paths using the application directory
/// as a base. This is necessary to avoid problems with the interpretation of
/// a relative path if current directory s changed at runtime.
std::string DIRECTORY_KEY_MAGIC{"director"};

/**
* Split the supplied string on semicolons.
*
Expand Down Expand Up @@ -118,8 +127,8 @@ std::vector<std::string> splitPath(const std::string &path) {
/// Private constructor for singleton class
ConfigServiceImpl::ConfigServiceImpl()
: m_pConf(nullptr), m_pSysConfig(nullptr), m_changed_keys(),
m_ConfigPaths(), m_AbsolutePaths(), m_strBaseDir(""),
m_PropertyString(""), m_properties_file_name("Mantid.properties"),
m_strBaseDir(""), m_PropertyString(""),
m_properties_file_name("Mantid.properties"),
#ifdef MPI_BUILD
// Use a different user properties file for an mpi-enabled build to avoid
// confusion if both are used on the same file system
Expand All @@ -140,25 +149,6 @@ ConfigServiceImpl::ConfigServiceImpl()

setBaseDirectory();

// Fill the list of possible relative path keys that may require conversion to
// absolute paths
m_ConfigPaths.emplace("mantidqt.python_interfaces_directory", true);
m_ConfigPaths.emplace("framework.plugins.directory", true);
m_ConfigPaths.emplace("pvplugins.directory", false);
m_ConfigPaths.emplace("mantidqt.plugins.directory", false);
m_ConfigPaths.emplace("instrumentDefinition.directory", true);
m_ConfigPaths.emplace("instrumentDefinition.vtpDirectory", true);
m_ConfigPaths.emplace("groupingFiles.directory", true);
m_ConfigPaths.emplace("maskFiles.directory", true);
m_ConfigPaths.emplace("colormaps.directory", true);
m_ConfigPaths.emplace("requiredpythonscript.directories", true);
m_ConfigPaths.emplace("pythonscripts.directory", true);
m_ConfigPaths.emplace("pythonscripts.directories", true);
m_ConfigPaths.emplace("python.plugins.directories", true);
m_ConfigPaths.emplace("user.python.plugins.directories", true);
m_ConfigPaths.emplace("datasearch.directories", true);
m_ConfigPaths.emplace("icatDownload.directory", true);

// attempt to load the default properties file that resides in the directory
// of the executable
std::string propertiesFilesList;
Expand Down Expand Up @@ -432,30 +422,6 @@ void ConfigServiceImpl::configureLogging() {
}
}

/**
* Searches the stored list for keys that have been loaded from the config file
* and may contain
* relative paths. Any it find are converted to absolute paths and stored
* separately
*/
void ConfigServiceImpl::convertRelativeToAbsolute() {
if (m_ConfigPaths.empty())
return;

m_AbsolutePaths.clear();
std::map<std::string, bool>::const_iterator send = m_ConfigPaths.end();
for (std::map<std::string, bool>::const_iterator sitr = m_ConfigPaths.begin();
sitr != send; ++sitr) {
std::string key = sitr->first;
if (!m_pConf->hasProperty(key))
continue;

std::string value(m_pConf->getString(key));
value = makeAbsolute(value, key);
m_AbsolutePaths.emplace(key, value);
}
}

/**
* Make a relative path or a list of relative paths into an absolute one.
* @param dir :: The directory to convert
Expand Down Expand Up @@ -513,25 +479,6 @@ std::string ConfigServiceImpl::makeAbsolute(const std::string &dir,
}
converted = Poco::Path(converted).makeDirectory().toString();

// C++ doesn't have a const version of operator[] for maps so I can't call
// that here
auto it = m_ConfigPaths.find(key);
bool required = false;
if (it != m_ConfigPaths.end()) {
required = it->second;
}
try {
if (required && !Poco::File(converted).exists()) {
g_log.debug() << "Required properties path \"" << converted
<< "\" in the \"" << key << "\" variable does not exist.\n";
converted = "";
}
} catch (Poco::FileException &) {
g_log.debug() << "Required properties path \"" << converted
<< "\" in the \"" << key << "\" variable does not exist.\n";
converted = "";
}

// Backward slashes cannot be allowed to go into our properties file
// Note this is a temporary fix for ticket #2445.
// Ticket #2460 prompts a review of our path handling in the config service.
Expand All @@ -545,7 +492,7 @@ std::string ConfigServiceImpl::makeAbsolute(const std::string &dir,
* The value of the key should be a semi-colon separated list of directories
*/
void ConfigServiceImpl::cacheDataSearchPaths() {
std::string paths = getString("datasearch.directories");
std::string paths = getString("datasearch.directories", true);
if (paths.empty()) {
m_DataSearchDirs.clear();
} else {
Expand Down Expand Up @@ -707,9 +654,6 @@ void ConfigServiceImpl::updateConfig(const std::string &filename,
if (update_caches) {
// Only configure logging once
configureLogging();
// Ensure that any relative paths given in the configuration file are
// relative to the correct directory
convertRelativeToAbsolute();
// Configure search paths into a specially saved store as they will be used
// frequently
cacheDataSearchPaths();
Expand Down Expand Up @@ -831,21 +775,20 @@ void ConfigServiceImpl::saveConfig(const std::string &filename) const {
*
* @param keyName :: The case sensitive name of the property that you need the
*value of.
* @param use_cache :: If true, the local cache of directory names is queried
*first.
* @param pathAbsolute :: If true then any key that looks like it contains
* a path has this path converted to an absolute path.
* @returns The string value of the property, or an empty string if the key
*cannot be found
*/
std::string ConfigServiceImpl::getString(const std::string &keyName,
bool use_cache) const {
if (use_cache) {
auto mitr = m_AbsolutePaths.find(keyName);
if (mitr != m_AbsolutePaths.end()) {
return (*mitr).second;
}
}
bool pathAbsolute) const {
if (m_pConf->hasProperty(keyName)) {
return m_pConf->getString(keyName);
std::string value = m_pConf->getString(keyName);
if (pathAbsolute &&
keyName.rfind(DIRECTORY_KEY_MAGIC) != std::string::npos) {
value = makeAbsolute(value, keyName);
}
return value;
}

g_log.debug() << "Unable to find " << keyName << " in the properties file"
Expand Down Expand Up @@ -988,12 +931,10 @@ void ConfigServiceImpl::setString(const std::string &key,
if (value == old)
return;

// Ensure we keep a correct full path
std::map<std::string, bool>::const_iterator itr = m_ConfigPaths.find(key);
if (itr != m_ConfigPaths.end()) {
m_AbsolutePaths[key] = makeAbsolute(value, key);
}
// Update the internal value
m_pConf->setString(key, value);

// Cache things that are accessed frequently
if (key == "datasearch.directories") {
cacheDataSearchPaths();
} else if (key == "instrumentDefinition.directory") {
Expand All @@ -1002,8 +943,6 @@ void ConfigServiceImpl::setString(const std::string &key,
appendDataSearchDir(value);
}

m_pConf->setString(key, value);

m_notificationCenter.postNotification(new ValueChanged(key, value, old));
m_changed_keys.insert(key);
}
Expand Down Expand Up @@ -1565,6 +1504,7 @@ void ConfigServiceImpl::appendDataSearchDir(const std::string &path) {
if (!isInDataSearchList(dirPath.toString())) {
auto newSearchString = Strings::join(std::begin(m_DataSearchDirs),
std::end(m_DataSearchDirs), ";");
newSearchString.append(path);
setString("datasearch.directories", newSearchString);
}
}
Expand Down Expand Up @@ -1600,7 +1540,7 @@ const std::string ConfigServiceImpl::getInstrumentDirectory() const {
*/
const std::string ConfigServiceImpl::getVTPFileDirectory() {
// Determine the search directory for XML instrument definition files (IDFs)
std::string directoryName = getString("instrumentDefinition.vtpDirectory");
std::string directoryName = getString("instrumentDefinition.vtp.directory");

if (directoryName.empty()) {
Poco::Path path(getAppDataDir());
Expand Down Expand Up @@ -1635,7 +1575,7 @@ void ConfigServiceImpl::cacheInstrumentPaths() {
#endif

// Determine the search directory for XML instrument definition files (IDFs)
std::string directoryName = getString("instrumentDefinition.directory");
std::string directoryName = getString("instrumentDefinition.directory", true);
if (directoryName.empty()) {
// This is the assumed deployment directory for IDFs, where we need to be
// relative to the
Expand Down
32 changes: 26 additions & 6 deletions Framework/Kernel/test/ConfigServiceTest.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,10 +350,32 @@ class ConfigServiceTest : public CxxTest::TestSuite {
TS_ASSERT_EQUALS(noseCountString, "");
}

void TestRelativeToAbsolute() {
// std::string path =
// ConfigService::Instance().getString("defaultsave.directory");
// TS_ASSERT( Poco::Path(path).isAbsolute() );
void testRelativeToAbsoluteForKeysWithCorrectIdentifier() {
auto &cfg = ConfigService::Instance();
cfg.setString("mantid.test.directory", "..");
cfg.setString("mantid.test.directories", "..");

TS_ASSERT(Poco::Path(cfg.getString("mantid.test.directory")).isAbsolute());
TS_ASSERT(
Poco::Path(cfg.getString("mantid.test.directories")).isAbsolute());
}

void testNoRelativeToAbsoluteForKeysWithoutCorrectIdentifier() {
auto &cfg = ConfigService::Instance();
cfg.setString("mantid.test.direc", "..");

TS_ASSERT(Poco::Path(cfg.getString("mantid.test.direc")).isRelative());
}

void testNoRelativeToAbsoluteForKeysOnRequest() {
auto &cfg = ConfigService::Instance();
cfg.setString("mantid.test.directory", "..");
cfg.setString("mantid.test.directories", "..");

TS_ASSERT(
Poco::Path(cfg.getString("mantid.test.directory", false)).isRelative());
TS_ASSERT(Poco::Path(cfg.getString("mantid.test.directories", false))
.isRelative());
}

void testAppendProperties() {
Expand Down Expand Up @@ -646,8 +668,6 @@ class ConfigServiceTest : public CxxTest::TestSuite {

// Returns all *root* keys, i.e. unique keys left of the first period
std::vector<std::string> keyVector = ConfigService::Instance().getKeys("");
// The 4 unique in the file and the ConfigService always sets a
// datasearch.directories key on creation
TS_ASSERT_EQUALS(keyVector.size(), 5);
}

Expand Down
11 changes: 6 additions & 5 deletions Framework/PythonInterface/mantid/kernel/src/Exports/ConfigService.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,12 @@ void export_ConfigService() {
"default.instrument is returned",
(arg("self"), arg("instrumentName") = boost::python::object()))
[return_value_policy<copy_const_reference>()])
.def(
"getString", &ConfigServiceImpl::getString,
getStringOverload("Returns the named key's value. If use_cache = "
"true [default] then relative paths->absolute",
(arg("self"), arg("key"), arg("use_cache") = true)))
.def("getString", &ConfigServiceImpl::getString,
getString_Overload(
"Returns the named key's value. If use_cache = "
"true [default] then relative paths->absolute",
(arg("self"), arg("key"), arg("pathAbsolute") = true)))

.def("setString", &ConfigServiceImpl::setString,
(arg("self"), arg("key"), arg("value")),
"Set the given property name. "
Expand Down

0 comments on commit d8205f1

Please sign in to comment.