-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
4847 logger singleton 3.0 #5110
Conversation
@kbenne can you do a quick walkthrough with some comments please? |
@@ -281,6 +281,7 @@ macro(MAKE_SWIG_TARGET NAME SIMPLENAME KEY_I_FILE I_FILES PARENT_TARGET PARENT_S | |||
set_target_properties(${swig_target} PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/ruby/") | |||
target_link_libraries(${swig_target} ${${PARENT_TARGET}_depends}) | |||
target_include_directories(${swig_target} SYSTEM PRIVATE ${RUBY_INCLUDE_DIRS}) | |||
target_compile_definitions(${swig_target} PRIVATE SHARED_OS_LIBS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SHARED_OS_LIBS needed because?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UtilitiesAPI preprocessor logic will not apply the required declspec if SHARED_OS_LIBS is not defined. That said, I think we could do away with entire SHARED_OS_LIBS definition with a bit of work. I don't think there is ever a configuration where we aren't using the openstudio.dll.
@@ -30,8 +30,8 @@ target_include_directories(pythonengine | |||
target_link_libraries( | |||
pythonengine | |||
PRIVATE | |||
openstudiolib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this just because of the logger? isn't it part of the minimal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we need this because of the Logger and yes Logger is part of the minimal utilities target, however minimal is a different compiled unit and since we want a single global Logger we cannot have it implmented in multiple compiled units. In fact, I think we can do away with the minimal target after this.
@@ -6,7 +6,7 @@ | |||
#endif | |||
|
|||
|
|||
#define UTILITIES_API | |||
%include <utilities/UtilitiesAPI.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? We need to dllexport the swig stuff inside the swig lib now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The swig generated code includes Logger.hpp which declares the Logger. We either need to link the Logger implementation into the swig generated code (like we have been doing with utilities minimal), or we need to use dllimport to signal that the implentation will come from a DLL. The problem with the former is that while linking to utilities minimal satisfies the linker, we had multiple implementations of the Logger floating around therefore it was not actually global. TLDR we need to declspec the Logger API everywhere that it is used.
{ | ||
|
||
friend class Singleton<LoggerSingleton>; | ||
friend class Logger; | ||
|
||
public: | ||
/// destructor, cleans up, writes xml file footers, etc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(what is this comment?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old cruft I guess. I think we can delete these lines.
|
||
public: | ||
/// destructor, cleans up, writes xml file footers, etc | ||
~LoggerSingleton(); | ||
~LoggerImpl(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the dtor really needed? it's empty. = default at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I agree.
5508f9f
to
7f2989b
Compare
I rebased onto develop after marking it "Ready for CI" so that CI tries to build it. Curious to see if the windows logger test is fixed |
Windows failing logger test in "classic" CLI
OpenStudio/src/cli/test/logger_test.rb Lines 1 to 15 in 800d735
So that Lines 83 to 90 in 800d735
|
Previously, we used the openstudio::Singleton template to create the singleton named Logger. This implementation suffered a bug described in #4847, where the singleton was not global across the ScriptEngine DLL boundary. This change is a non templated implementation of Logger. The design is identical to openstudio::Singleton<LoggerSingleton>, but does not suffer the non-global issue. ref #4847
UtilitiesAPI.hpp needs to be included by SWIG so that the correct declspecs are used for the global logger. Previously, UTILITIES_API was just #define OPENSTUDIO_API, therefore the dll export decorator was not used.
7f2989b
to
c162c04
Compare
/// Explicitly instantiate and export LoggerSingleton Singleton template instance | ||
/// so that the same instance is shared between the DLL's that link to Utilities.dll | ||
UTILITIES_TEMPLATE_EXT template class UTILITIES_API openstudio::Singleton<LoggerSingleton>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, openstudio::Singleton should be removed everywhere. It was needed a long time ago, but the same can be achieved with a Meyers' Singleton like you did (static ref& instance()
)
/// Explicitly instantiate and export LoggerSingleton Singleton template instance | ||
/// so that the same instance is shared between the DLL's that link to Utilities.dll | ||
UTILITIES_TEMPLATE_EXT template class UTILITIES_API openstudio::Singleton<LoggerSingleton>; | ||
static LoggerImpl& instance() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this file weird for two reasons:
- Why not create a Logger_Impl.hpp to put the impl inside
- put it in namespace detail
- Rename class to Logger_Impl).
- => That's the Impl pattern we have all over openstudio
- Why is this static method definition in the header and not the cpp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd also like to follow Meyers' singleton pattern. Something feels a bit off here for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to keep the name of the thing that has the ::instance
method as Logger
otherwise we would break third party Measures. The question then becomes what do you call the thing that is returned by Logger::instance
? I felt like LoggerSingleton was a really unfortunate name because it isn't a singleton (and the name at least confused me at first) so I renamed LoggerSingleton as LoggerImpl, which isn't great because it implies the Impl pattern that is used throughout OpenStudio, which it is not. I do think we could call it something else though, I just couldn't come up with a better name. I would be ok with renaming LoggerImpl
as Logger_Impl
and moving it under namesapce detail. Note that it is still a public thing, but then again so are all of our other Impls.
I don't think there's a particular reason why the static method definition is in the header and not the cpp. That is how the original code was and it didn't bother me enough to change.
The Meyers' singleton had one unfortunate side effect that was messing with us in this case, and that is it makes it easy to end up with multiple definitions. The singleton that I used here (phoenix singleton?) will yield a linker error if you don't link the one (and only) library where the singleton is defined. This helped me work through the issue. I also believe that we could use Meyers' singleton and move the defintion into the cpp and accomplish the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You return a Logger, cf #5119
class Logger {
public:
static Logger& instance() {
static Logger logger;
return logger;
}
Logger(const Logger& other) = delete;
Logger(Logger&& other) = delete;
Logger& operator=(const Logger&) = delete;
Logger& operator=(Logger&&) = delete;
private:
Logger();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do follow the point about the Phoenix Singleton though.
in #5119 I moved the instance() definition to the cpp file already, do you think that's enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I do think that is enough. If the new test that I wrote is passing (on windows) then I am convinced.
CI Results for c162c04:
|
@@ -16,12 +16,14 @@ namespace keywords = boost::log::keywords; | |||
|
|||
namespace openstudio { | |||
|
|||
std::shared_ptr<LoggerImpl> Logger::obj = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that's the bit that actually makes it global right?
Supperseded by #5119 |
@jmarrec I think this should address the core issue that the logger is not global across the script engines. I think there remains a little bit of work.
SHARED_OS_LIBS
. What do you think?if Windows
conditions that attempted to work around the issue.