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

Remove static globals #31

Merged
merged 4 commits into from Sep 22, 2015

Conversation

Projects
None yet
3 participants
@jeffdiclemente
Contributor

jeffdiclemente commented Aug 25, 2015

This addresses jeffdiclemente#2, which is concerned with removal of static globals and other related code changes.

This pull request obsoletes #28, which was primarily focused on the review of the high level design.

Important Notes:

  • Auto-loading and ModuleSettings are in a state of transition. I am leaving it unfinished for now. Currently, bundles are auto-installed (i.e.auto-installed bundles have to be explicitly started/stopped), instead of auto-loaded. Requirements and/or guidance on how to transition this would be great.
  • All new code uses OSGi nomenclature where applicable in anticipation of submitting code for jeffdiclemente#1.

jeffdiclemente added some commits Jul 1, 2015

Update README.md
Added disclaimer describing the purpose of this repository.

Signed-off-by: The MathWorks, Inc. Roy.Lurie@mathworks.com
Remove static globals
This includes a few other changes required to be able to remove static globals:

- "Framework" and "Framework Factory" classes as a single entry point for clients to initialize CppMicroServices.
- A minimal lifecycle for bundles, consisting solely of "install", "start", "stop" and "uninstall".
- Bundles are installed first and then loaded into the process only on bundle activation.
- Transition to declaring the bundle name in the bundle's manifest file.
- Auto-loading needs to be either factored out into a service or modified to allow auto-installing of bundles.

Closes #2

Signed-off-by: The MathWorks, Inc. Roy.Lurie@mathworks.com
@jeffdiclemente

This comment has been minimized.

Show comment
Hide comment
@jeffdiclemente

jeffdiclemente Aug 25, 2015

Contributor

I'm not sure why the AppVeyor build fails. It has consistently done so with the following output:

Build started
git clone -q https://github.com/saschazelzer/CppMicroServices.git C:\projects\cppmicroservices
git fetch -q origin +refs/pull/31/merge:
git checkout -qf FETCH_HEAD
Specify a project or solution file. The directory does not contain a project or solution file.

Is this an issue on the AppVeyor side or an issue on my end?

Contributor

jeffdiclemente commented Aug 25, 2015

I'm not sure why the AppVeyor build fails. It has consistently done so with the following output:

Build started
git clone -q https://github.com/saschazelzer/CppMicroServices.git C:\projects\cppmicroservices
git fetch -q origin +refs/pull/31/merge:
git checkout -qf FETCH_HEAD
Specify a project or solution file. The directory does not contain a project or solution file.

Is this an issue on the AppVeyor side or an issue on my end?

Show outdated Hide outdated core/doc/snippets/uServices-modulecontext/main.cpp
@@ -16,9 +16,7 @@ void RetrieveModuleContext()
//! [GetModuleContext]
//! [InitializeModule]

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

Remove comment

@WayneBailey
Show outdated Hide outdated core/doc/snippets/uServices-modulecontext/main.cpp
US_INITIALIZE_MODULE
//! [InitializeModule]

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

Remove comment

@WayneBailey

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 8, 2015

Contributor

The comment should stay since I mistakenly removed

#include <usModuleInitialization.h>
US_INITIALIZE_MODULE

which I believe should remain in since it illustrates the connection between declaring a bundle and getting its bundle context.

@jeffdiclemente

jeffdiclemente Sep 8, 2015

Contributor

The comment should stay since I mistakenly removed

#include <usModuleInitialization.h>
US_INITIALIZE_MODULE

which I believe should remain in since it illustrates the connection between declaring a bundle and getting its bundle context.

@@ -38,11 +37,11 @@ void parseComponentDefinition(std::istream&)
{
}
void extenderPattern()
void extenderPattern(ModuleContext* moduleCtx)
{
//! [2]
// Get all loaded modules

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

I assume all comments mentioning “loaded modules” will be changed to "installed bundle" when the nomenclature is changed.

@WayneBailey

WayneBailey Aug 27, 2015

I assume all comments mentioning “loaded modules” will be changed to "installed bundle" when the nomenclature is changed.

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Aug 31, 2015

Contributor

Yes, nomenclature changes will correct such occurrences.

FYI; "loaded" would equate to "started". "Install" and "Uninstall" are new states introduced by these changes.

@jeffdiclemente

jeffdiclemente Aug 31, 2015

Contributor

Yes, nomenclature changes will correct such occurrences.

FYI; "loaded" would equate to "started". "Install" and "Uninstall" are new states introduced by these changes.

Show outdated Hide outdated core/doc/snippets/uServices-staticmodules/main.cpp
@@ -34,6 +41,4 @@ int main(int /*argc*/, char* /*argv*/[])
}
//! [InitializeExecutable]

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

Remove comment

@WayneBailey
Show outdated Hide outdated core/doc/snippets/uServices-staticmodules/main.cpp
@@ -34,6 +41,4 @@ int main(int /*argc*/, char* /*argv*/[])
}
//! [InitializeExecutable]
#include <usModuleInitialization.h>
US_INITIALIZE_MODULE
//! [InitializeExecutable]

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

Remove comment

@WayneBailey
Show outdated Hide outdated core/examples/driver/main.cpp
@@ -75,12 +75,20 @@ int main(int /*argc*/, char** /*argv*/)
{
char cmd[256];
std::vector<std::string> availableModules = GetExampleModules();
FrameworkFactory factory;
Framework* framework = factory.newFramework(std::map<std::string, std::string>());

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

A comment regarding the argument would be helpful.

@WayneBailey

WayneBailey Aug 27, 2015

A comment regarding the argument would be helpful.

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

There should be an overload in the FrameworkFactory class of the newFramework method taking no arguments. See my comments for this class.

@saschazelzer

saschazelzer Sep 8, 2015

Member

There should be an overload in the FrameworkFactory class of the newFramework method taking no arguments. See my comments for this class.

Show outdated Hide outdated core/examples/driver/main.cpp
availableModules.end());
}
/* It is possible to install a bundle based on its name, if its

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

This limitation is not described in the comments for InstallBundle. Why does the bundle name need to be the same as the file that contains it (assuming this is what this comment implies)?

@WayneBailey

WayneBailey Aug 27, 2015

This limitation is not described in the comments for InstallBundle. Why does the bundle name need to be the same as the file that contains it (assuming this is what this comment implies)?

Show outdated Hide outdated core/examples/driver/main.cpp
for (std::vector<std::string>::const_iterator iter = availableModules.begin();
iter != availableModules.end(); ++iter)
{
frameworkContext->InstallBundle(LIB_PATH + PATH_SEPARATOR + LIB_PREFIX + (*iter) + LIB_EXT + "/" + (*iter));

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

Where is this location syntax (i.e. /) defined?

@WayneBailey

WayneBailey Aug 27, 2015

Where is this location syntax (i.e. /) defined?

@@ -228,39 +188,31 @@ int main(int /*argc*/, char** /*argv*/)
long int id = -1;
ss >> id;
// TODO: the "system bundle" is 0 in the OSGi spec. Change this once CppMicroServices is
// inline with the spec.
if (id == 1)
{
std::cout << "Info: Unloading not possible" << std::endl;

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

This message can be more specific. Mention system bundle.

@WayneBailey

WayneBailey Aug 27, 2015

This message can be more specific. Mention system bundle.

@@ -288,7 +240,7 @@ int main(int /*argc*/, char** /*argv*/)
{
std::cout << std::right << std::setw(2) << (*moduleIter)->GetModuleId() << std::left << " | ";
std::cout << std::setw(20) << (*moduleIter)->GetName() << " | ";
std::cout << std::setw(9) << ((*moduleIter)->IsLoaded() ? "LOADED" : "UNLOADED");
std::cout << std::setw(9) << ((*moduleIter)->IsLoaded() ? "ACTIVE" : "RESOLVED");

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

Is there a "resolved" state given there's no fully supported life cycle yet?

@WayneBailey

WayneBailey Aug 27, 2015

Is there a "resolved" state given there's no fully supported life cycle yet?

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Aug 31, 2015

Contributor

It depends on what's trying to be accomplished by the status information displayed by this code.

If we want to show users bundle state with a fully supported lifecycle, "resolved" and "active" would be acceptable. Otherwise, "resolved" would be "stopped" and "active" would be "started" given our current level of lifecycle support.

Ultimately, IsLoaded() would probably be replaced by something like GetState(), which is a standard OSGi defined function, when there is a fully supported lifecycle.

@jeffdiclemente

jeffdiclemente Aug 31, 2015

Contributor

It depends on what's trying to be accomplished by the status information displayed by this code.

If we want to show users bundle state with a fully supported lifecycle, "resolved" and "active" would be acceptable. Otherwise, "resolved" would be "stopped" and "active" would be "started" given our current level of lifecycle support.

Ultimately, IsLoaded() would probably be replaced by something like GetState(), which is a standard OSGi defined function, when there is a fully supported lifecycle.

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I suggest to keep it like this for now and agree with Jeff that we need to introduce a GetState() method later instead.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I suggest to keep it like this for now and agree with Jeff that we need to introduce a GetState() method later instead.

* Framework instances are created using a FrameworkFactory. The methods of this class can be
* used to manage and control the created framework instance.
*
* @remarks This class is thread-safe.

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

Always thread safe or depending on build option? If not build option, why?

@WayneBailey

WayneBailey Aug 27, 2015

Always thread safe or depending on build option? If not build option, why?

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Such remarks are all over the place and they are true if the related build option is enabled. Otherwise they are certainly not. We could just add a general remark to the documentation of the build option.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Such remarks are all over the place and they are true if the related build option is enabled. Otherwise they are certainly not. We could just add a general remark to the documentation of the build option.

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

I agree with documenting this in one place. I've made note of this in BuildInstructions.md

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

I agree with documenting this in one place. I've made note of this in BuildInstructions.md

Show outdated Hide outdated core/include/usFramework.h
*
* <p>
* The following steps are taken to start this Framework:
* -# If this Framework is not in the STARTING state, initialize this Framework.

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

So if I call this Start method, then I don't need to call the Init method above?
If so, then why the Init method?

@WayneBailey

WayneBailey Aug 27, 2015

So if I call this Start method, then I don't need to call the Init method above?
If so, then why the Init method?

@@ -0,0 +1,61 @@
/*=============================================================================

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

It would be helpful to describe why FrameworkPrivate is necessary.

@WayneBailey

WayneBailey Aug 27, 2015

It would be helpful to describe why FrameworkPrivate is necessary.

case ModuleEvent::UNLOADED: return os << "UNLOADED";
case ModuleEvent::LOADING: return os << "LOADING";
case ModuleEvent::UNLOADING: return os << "UNLOADING";
case ModuleEvent::LOADED: return os << "LOADED";

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

Why are events other than installed and uninstalled needed?

@WayneBailey

WayneBailey Aug 27, 2015

Why are events other than installed and uninstalled needed?

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Aug 31, 2015

Contributor

Clients can still start and stop bundles. LOADED/LOADING equates to STARTED/STARTING and UNLOADING/UNLOADED equates to STOPPED/STOPPING now. These are all valid bundle states given the current functionality of CppMicroServices.

Nomenclature changes will make this clearer.

@jeffdiclemente

jeffdiclemente Aug 31, 2015

Contributor

Clients can still start and stop bundles. LOADED/LOADING equates to STARTED/STARTING and UNLOADING/UNLOADED equates to STOPPED/STOPPING now. These are all valid bundle states given the current functionality of CppMicroServices.

Nomenclature changes will make this clearer.

Show outdated Hide outdated core/src/util/usFrameworkFactory.cpp
{
}
Framework* FrameworkFactory::newFramework(std::map<std::string, std::string> configuration)

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

Where are valid configuration options defined?
Where ever such configurations are referenced there should be a pointer to where these options are defined.

@WayneBailey

WayneBailey Aug 27, 2015

Where are valid configuration options defined?
Where ever such configurations are referenced there should be a pointer to where these options are defined.

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I agree, we should add static const std::string members to the Frameworkclass defining the config options known to the framework.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I agree, we should add static const std::string members to the Frameworkclass defining the config options known to the framework.

"If control enters the declaration concurrently while the variable is being initialized,
the concurrent execution shall wait for completion of the initialization."
However, even if a C++11 supported compiler isn't used, thread safe initialization

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

If thread safety isn't an issue, why mention it?
Is there a compelling reason why someone other than the framework would like to use it and thus make thread safety an issue? If so what's that reason?

@WayneBailey

WayneBailey Aug 27, 2015

If thread safety isn't an issue, why mention it?
Is there a compelling reason why someone other than the framework would like to use it and thus make thread safety an issue? If so what's that reason?

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Aug 31, 2015

Contributor

I mention it because thread-safety will become an issue if the LogMsg interface is made public, as shown in branch 25-module-lifecycle-enhancements in this commit

@jeffdiclemente

jeffdiclemente Aug 31, 2015

Contributor

I mention it because thread-safety will become an issue if the LogMsg interface is made public, as shown in branch 25-module-lifecycle-enhancements in this commit

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I think we can avoid potential problems by making sure we call Logger::instance at least once during static initialization of the CppMicroServices class. The loader will hold a "global loader mutex" which avoids any race conditions.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I think we can avoid potential problems by making sure we call Logger::instance at least once during static initialization of the CppMicroServices class. The loader will hold a "global loader mutex" which avoids any race conditions.

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

On a second thought, having a static Logger instance is in conflict with the ability to create multiple Framework instances. So this needs a redesign anyway, possibly relying on a ModuleContext instance. I suggest to keep the code as is and add yet another issue to tackle this.

@saschazelzer

saschazelzer Sep 8, 2015

Member

On a second thought, having a static Logger instance is in conflict with the ability to create multiple Framework instances. So this needs a redesign anyway, possibly relying on a ModuleContext instance. I suggest to keep the code as is and add yet another issue to tackle this.

Show outdated Hide outdated core/src/util/usSharedLibrary.cpp
{ // Testing for file extension isn't the most robust way to test
// for file type. Furthermore, the existence of this function
// is out of place; why test for a shared library if this class
// is supposed to represent a shared library?

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

Good question. So what are you going to do about it?

@WayneBailey

WayneBailey Aug 27, 2015

Good question. So what are you going to do about it?

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

We can do a more robust check check later using the object format parser in the 25-module-lifecycle-enhancements branch (I will create a new issue)

@saschazelzer

saschazelzer Sep 8, 2015

Member

We can do a more robust check check later using the object format parser in the 25-module-lifecycle-enhancements branch (I will create a new issue)

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 8, 2015

Contributor

@saschazelzer, I agree. In the meantime, I am moving this function out of usSharedLibrary.cpp (because it doesn't make much logical sense) and into usUtils.cpp.

@jeffdiclemente

jeffdiclemente Sep 8, 2015

Contributor

@saschazelzer, I agree. In the meantime, I am moving this function out of usSharedLibrary.cpp (because it doesn't make much logical sense) and into usUtils.cpp.

This comment has been minimized.

@saschazelzer

saschazelzer Sep 9, 2015

Member

Okay.

@saschazelzer
@@ -0,0 +1,3 @@
{
"module.name" : "TestModuleA"

This comment has been minimized.

@WayneBailey

WayneBailey Aug 27, 2015

No module.version needed?

@WayneBailey

WayneBailey Aug 27, 2015

No module.version needed?

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Aug 31, 2015

Contributor

Yes, module.version isn't required for this test bundle.

@jeffdiclemente

jeffdiclemente Aug 31, 2015

Contributor

Yes, module.version isn't required for this test bundle.

@saschazelzer saschazelzer self-assigned this Sep 8, 2015

@saschazelzer

This comment has been minimized.

Show comment
Hide comment
@saschazelzer

saschazelzer Sep 8, 2015

Member

Concerning the AppVeyor issue, I am not sure what's going on. Lets ignore this for now, we can fix the build later.

Member

saschazelzer commented Sep 8, 2015

Concerning the AppVeyor issue, I am not sure what's going on. Lets ignore this for now, we can fix the build later.

@@ -7,15 +7,22 @@ The following properties are always set by the C++ Micro Services library and ca
the module author:
* `module.id` - The unique id of the module (type `long`)
* `module.name` - The human readable name of the module (type `std::string`)

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I believe I understand why you made this change. Still, I would like to see the module.name property added by the framework, such that for basic bundles no custom manifest.json file is required.

We should be able to use the US_MODULE_INITIALIZATION macro to inject a string field into the custom section we were talking about in the General Discussion document. The existing object format parsers in branch 25-module-lifecycle-enhancements can then extract that information. Because this requires some more coding, I suggest to keep your changes for the sake of merging them rather sooner than later. I will create a separate issue to investigate this after the merge.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I believe I understand why you made this change. Still, I would like to see the module.name property added by the framework, such that for basic bundles no custom manifest.json file is required.

We should be able to use the US_MODULE_INITIALIZATION macro to inject a string field into the custom section we were talking about in the General Discussion document. The existing object format parsers in branch 25-module-lifecycle-enhancements can then extract that information. Because this requires some more coding, I suggest to keep your changes for the sake of merging them rather sooner than later. I will create a separate issue to investigate this after the merge.

Show outdated Hide outdated core/examples/driver/main.cpp
FrameworkFactory factory;
Framework* framework = factory.newFramework(std::map<std::string, std::string>());
framework->init();
framework->Start();

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

As Wayne asked somewhere else, is there a reason for separate init and Start calls? Can we get rid of the init?

@saschazelzer

saschazelzer Sep 8, 2015

Member

As Wayne asked somewhere else, is there a reason for separate init and Start calls? Can we get rid of the init?

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 8, 2015

Contributor

The init() and Start() functions are defined in OSGi and from what I can tell, its to facilitate the Start Level Service Specification within OSGi.

Since CppMicroServices doesn't support the Start Level Service specification, I don't see a compelling reason to keep init(). I will remove it.

@jeffdiclemente

jeffdiclemente Sep 8, 2015

Contributor

The init() and Start() functions are defined in OSGi and from what I can tell, its to facilitate the Start Level Service Specification within OSGi.

Since CppMicroServices doesn't support the Start Level Service specification, I don't see a compelling reason to keep init(). I will remove it.

@@ -75,12 +75,20 @@ int main(int /*argc*/, char** /*argv*/)
{

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Trying to run the usCoreExamplesDriver executable on my Fedora 22 box crashes with the following output:

[sascha@bigeye-nb bin]$ ./usCoreExamplesDriver 
ServiceTracker<S,TTT>::Open: 1
terminate called after throwing an instance of 'std::runtime_error'
  what():  module.name is not defined in the bundle manifest.
Aborted (core dumped)

Does it run for you?

@saschazelzer

saschazelzer Sep 8, 2015

Member

Trying to run the usCoreExamplesDriver executable on my Fedora 22 box crashes with the following output:

[sascha@bigeye-nb bin]$ ./usCoreExamplesDriver 
ServiceTracker<S,TTT>::Open: 1
terminate called after throwing an instance of 'std::runtime_error'
  what():  module.name is not defined in the bundle manifest.
Aborted (core dumped)

Does it run for you?

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 8, 2015

Contributor

I saw this issue after posting the pull request.
I have fixed this issue and will submit the fix to this pull request.

@jeffdiclemente

jeffdiclemente Sep 8, 2015

Contributor

I saw this issue after posting the pull request.
I have fixed this issue and will submit the fix to this pull request.

Show outdated Hide outdated core/include/usFramework.h
*
* @throw std::runtime_error If this Framework could not be initialized.
*/
void init(void);

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

The general policy for method names is camel case. Please change to Init(void).

@saschazelzer

saschazelzer Sep 8, 2015

Member

The general policy for method names is camel case. Please change to Init(void).

Show outdated Hide outdated core/include/usFramework.h
Framework(const Framework&);
Framework operator=(const Framework&);
FrameworkPrivate* f;

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Naming conventions for private impl pointers is d. Please use FrameworkPrivate* d.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Naming conventions for private impl pointers is d. Please use FrameworkPrivate* d.

Show outdated Hide outdated core/include/usFrameworkFactory.h
* Create a new Framework instance.
*
* @param configuration The framework properties to configure the new framework instance. If framework properties
* are not provided by the configuration argument, the created framework instance must use some reasonable

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Please avoid "must" clauses. These are spec / interface wording but we do not expect people to write their own FrameworkFactory implementations.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Please avoid "must" clauses. These are spec / interface wording but we do not expect people to write their own FrameworkFactory implementations.

Show outdated Hide outdated core/include/usFrameworkFactory.h
* @param configuration The framework properties to configure the new framework instance. If framework properties
* are not provided by the configuration argument, the created framework instance must use some reasonable
* default configuration. The created framework instance must copy any information needed from the specified
* configuration argument since the configuration argument can be changed after the framework instance has been created.

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Remove statement about copying config arguments, we do that anyway.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Remove statement about copying config arguments, we do that anyway.

Show outdated Hide outdated core/include/usFrameworkFactory.h
*
* @return A new, configured Framework instance.
*/
Framework* newFramework(std::map<std::string, std::string> configuration);

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Camel case method naming NewFramework.

Please add an overload taking no arguments.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Camel case method naming NewFramework.

Please add an overload taking no arguments.

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

We probably should use unique / shared pointer for the framework and module instances. Previously, the Module pointers lived as long as the program, but with the new life cycle they should be managed. I will create a new issue for that.

@saschazelzer

saschazelzer Sep 8, 2015

Member

We probably should use unique / shared pointer for the framework and module instances. Previously, the Module pointers lived as long as the program, but with the new life cycle they should be managed. I will create a new issue for that.

Show outdated Hide outdated core/include/usFrameworkFactory.h
#define USFRAMEWORKFACTORY_H
#include "usGlobalConfig.h"
#include "usFramework.h"

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Remove the include statement and forward declare the Framework class.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Remove the include statement and forward declare the Framework class.

Show outdated Hide outdated core/include/usGetModuleContext.h
@@ -29,10 +29,12 @@
#endif
#include <usGlobalConfig.h>

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

This is not needed anymore.

@saschazelzer

saschazelzer Sep 8, 2015

Member

This is not needed anymore.

Show outdated Hide outdated core/include/usGetModuleContext.h
@@ -29,10 +29,12 @@
#endif
#include <usGlobalConfig.h>
#include <usModuleRegistry.h>
#include <usModule.h>

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

This is not needed anymore.

@saschazelzer

saschazelzer Sep 8, 2015

Member

This is not needed anymore.

Show outdated Hide outdated core/include/usModule.h
* @return <code>true</code> if the module is <code>LOADED</code>
* <code>false</code> if it is in any other state.
*/
virtual bool IsLoaded() const;

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I believe this and all the following methods do not need to be virtual. Please investigate.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I believe this and all the following methods do not need to be virtual. Please investigate.

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

The Framework class inherits from Module since the Framework "is a" Bundle (i.e. its the system bundle). Clients can get a Module instance from the Framework which represents the system bundle and that instance needs to behave differently under certain cases (e.g. when Uninstall(), GetLocation(), Start() and Stop() are called). Considering this and the OSGi's specification for the Framework class, I turned the Module class into a base class so that Framework could derive from it. That's why they are all virtual.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

The Framework class inherits from Module since the Framework "is a" Bundle (i.e. its the system bundle). Clients can get a Module instance from the Framework which represents the system bundle and that instance needs to behave differently under certain cases (e.g. when Uninstall(), GetLocation(), Start() and Stop() are called). Considering this and the OSGi's specification for the Framework class, I turned the Module class into a base class so that Framework could derive from it. That's why they are all virtual.

This comment has been minimized.

@saschazelzer

saschazelzer Sep 9, 2015

Member

I was probably not clear about this. I understand the inheritance relationship, but the Framework class only overrides a small subset (Start, Stop, Uninstall, ?) so my suggestion was to remove virtual from all methods which do not need to be overridden by the Framework class.

@saschazelzer

saschazelzer Sep 9, 2015

Member

I was probably not clear about this. I understand the inheritance relationship, but the Framework class only overrides a small subset (Start, Stop, Uninstall, ?) so my suggestion was to remove virtual from all methods which do not need to be overridden by the Framework class.

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

Understood. No problem, I'll convert the other functions back to non-virtual.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

Understood. No problem, I'll convert the other functions back to non-virtual.

Show outdated Hide outdated core/include/usModule.h
void Uninit();
protected:
// purposely not implemented

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Why are the disabled copy constructor and assignment operator protected instead of private?

@saschazelzer

saschazelzer Sep 8, 2015

Member

Why are the disabled copy constructor and assignment operator protected instead of private?

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

There is no reason. This may have happened during development and I forgot to move them back under private. Good catch.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

There is no reason. This may have happened during development and I forgot to move them back under private. Good catch.

Show outdated Hide outdated core/include/usModuleContext.h
* The following steps are taken to get the service object:
* <ol>
* <li>If the service has been unregistered, <code>0</code> is returned.
* <li>The context module's use count for this service is incremented by

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I think we will not do URL parsing and downloading in the near future. We should probably say "Typically an absolute file system path" or similar.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I think we will not do URL parsing and downloading in the near future. We should probably say "Typically an absolute file system path" or similar.

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

I agree. Based on your and Wayne's feedback, I've revised the entire comment block to

  /**
   * Installs a bundle from the specified location.
   *
   * The following steps are required to install a bundle:
   * -# If a bundle containing the same location identifier is already installed, the Bundle object for that 
   *    bundle is returned.
   * -# The bundle's associated resources are allocated. The associated resources minimally consist of a
   *    unique identifier and a persistent storage area if the platform has file system support. If this step
   *    fails, a std::runtime_error is thrown.
   * -# A bundle event of type <code>BundleEvent::INSTALLED</code> is fired.
   * -# The Bundle object for the newly or previously installed bundle is returned.
   *
   * @remarks A location identifier is defined as an absolute path to a shared library or executable file
   * followed by a slash (/) and the bundle's name.
   * 
   * For example: 
   * -# <code>InstallBundle("/path/to/bundle/foo.so/foo");</code>
   * -# <code>InstallBundle("/path/to/bundle/foo.so/my_static_bundle");</code>
   *
   * @param location The location identifier of the bundle to install.
   * @return The Bundle object of the installed bundle.
   * @throws std::runtime_error If the installation failed.
   */
@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

I agree. Based on your and Wayne's feedback, I've revised the entire comment block to

  /**
   * Installs a bundle from the specified location.
   *
   * The following steps are required to install a bundle:
   * -# If a bundle containing the same location identifier is already installed, the Bundle object for that 
   *    bundle is returned.
   * -# The bundle's associated resources are allocated. The associated resources minimally consist of a
   *    unique identifier and a persistent storage area if the platform has file system support. If this step
   *    fails, a std::runtime_error is thrown.
   * -# A bundle event of type <code>BundleEvent::INSTALLED</code> is fired.
   * -# The Bundle object for the newly or previously installed bundle is returned.
   *
   * @remarks A location identifier is defined as an absolute path to a shared library or executable file
   * followed by a slash (/) and the bundle's name.
   * 
   * For example: 
   * -# <code>InstallBundle("/path/to/bundle/foo.so/foo");</code>
   * -# <code>InstallBundle("/path/to/bundle/foo.so/my_static_bundle");</code>
   *
   * @param location The location identifier of the bundle to install.
   * @return The Bundle object of the installed bundle.
   * @throws std::runtime_error If the installation failed.
   */

This comment has been minimized.

@saschazelzer

saschazelzer Sep 9, 2015

Member

Could you explain why the bundle name at the end is needed?

@saschazelzer

saschazelzer Sep 9, 2015

Member

Could you explain why the bundle name at the end is needed?

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

It allows static bundles to be installed explicitly into the Framework. This assumes we are adhering to OSGi's spec for the InstallBundle API.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

It allows static bundles to be installed explicitly into the Framework. This assumes we are adhering to OSGi's spec for the InstallBundle API.

Show outdated Hide outdated core/include/usModuleInfo.h
@@ -27,6 +27,7 @@
#include <usCoreConfig.h>
#include <string>
#include <map>

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I think this is not needed.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I think this is not needed.

Show outdated Hide outdated core/include/usModuleSettings.h
@@ -43,11 +45,15 @@ US_BEGIN_NAMESPACE
* - \e US_AUTOLOAD_PATHS A ':' (Unix) or ';' (Windows) separated list of paths
* from which modules should be auto-loaded.
*
* \deprecated Use FrameworkFactory::newFramework(std::map<std::string, std::string> configuration) to configure the framework.

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Since this is deprecated now, we would need standardized framework properties for threading, logging, and storage configurations. These chould probably go into the Framework class and their values should start with org.cppmicroservices. Do not use org.osgi because of trademarks issues.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Since this is deprecated now, we would need standardized framework properties for threading, logging, and storage configurations. These chould probably go into the Framework class and their values should start with org.cppmicroservices. Do not use org.osgi because of trademarks issues.

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

That sounds good. I'll create an issue to standardize the framework properties for theading, logging and storage along with an initial set of default framework property values. If you agree, I'll make this changes as a separate PR.

For storage configuration, can we not use the OSGi org.osgi.framework.storage property? This is a standard OSGi property used to define a Framework's persistent storage location. Its already used in this PR (usModuleContext.cpp - line 182).

Could you elaborate on the trademark issues?

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

That sounds good. I'll create an issue to standardize the framework properties for theading, logging and storage along with an initial set of default framework property values. If you agree, I'll make this changes as a separate PR.

For storage configuration, can we not use the OSGi org.osgi.framework.storage property? This is a standard OSGi property used to define a Framework's persistent storage location. Its already used in this PR (usModuleContext.cpp - line 182).

Could you elaborate on the trademark issues?

This comment has been minimized.

@saschazelzer

saschazelzer Sep 9, 2015

Member

Because the project is not officially affiliated with the OSGi consortium, I would like to avoid any troubles regarding the re-use of the term "OSGi". Maybe it is not an issue for string literals - I didn't consult a lawyer. But I know one project which had to change its name because the OSGi guys knocked on its doors. Granted, it was the project name itself but still...

@saschazelzer

saschazelzer Sep 9, 2015

Member

Because the project is not officially affiliated with the OSGi consortium, I would like to avoid any troubles regarding the re-use of the term "OSGi". Maybe it is not an issue for string literals - I didn't consult a lawyer. But I know one project which had to change its name because the OSGi guys knocked on its doors. Granted, it was the project name itself but still...

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

Ok, understood. I will steer clear of "org.osgi".

I will change "org.osgi.framework.storage" to, minimally, replace "org.osgi" with "org.cppmicroservices".

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

Ok, understood. I will steer clear of "org.osgi".

I will change "org.osgi.framework.storage" to, minimally, replace "org.osgi" with "org.cppmicroservices".

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 11, 2015

Contributor

@saschazelzer I've created an issue to track the implementation of framework properties. Please review and let me know what you think. Thanks.

@jeffdiclemente

jeffdiclemente Sep 11, 2015

Contributor

@saschazelzer I've created an issue to track the implementation of framework properties. Please review and let me know what you think. Thanks.

Show outdated Hide outdated core/src/CMakeLists.txt
@@ -57,9 +60,9 @@ set(_srcs
set(_private_headers
util/usAtomicInt_p.h
util/usFrameworkPrivate_p.h

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Please rename to usFrameworkPrivate.h. The _p is only used if there is no Private suffix in the name already.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Please rename to usFrameworkPrivate.h. The _p is only used if there is no Private suffix in the name already.

Show outdated Hide outdated core/src/module/usModule.cpp
@@ -119,15 +121,37 @@ bool Module::IsLoaded() const
void Module::Start()
{
MutexLockingStrategy::Lock(this->d);

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

We need to review the locking strategy here. Framework locks must not be held when calling client code, e.g. the module activator and module changes listeners below.

You can "opt out" and defer this by adding some info about this to the existing threading issues.

@saschazelzer

saschazelzer Sep 8, 2015

Member

We need to review the locking strategy here. Framework locks must not be held when calling client code, e.g. the module activator and module changes listeners below.

You can "opt out" and defer this by adding some info about this to the existing threading issues.

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 10, 2015

Contributor

This multi-threading issue will have to be fixed for 3.0 sooner or later. I'd prefer sooner, so I will fix this one now within this PR.

@jeffdiclemente

jeffdiclemente Sep 10, 2015

Contributor

This multi-threading issue will have to be fixed for 3.0 sooner or later. I'd prefer sooner, so I will fix this one now within this PR.

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 11, 2015

Contributor

After looking at the problem, I don't see a good solution, that doesn't hold a lock while calling into client code, which doesn't require a significant change.

I removed the use of MutexLockingStrategy and replaced it with MutexLock and Mutex since its simpler.

I will "opt out" and add information to the multithreading issue so we can re-visit it.

@jeffdiclemente

jeffdiclemente Sep 11, 2015

Contributor

After looking at the problem, I don't see a good solution, that doesn't hold a lock while calling into client code, which doesn't require a significant change.

I removed the use of MutexLockingStrategy and replaced it with MutexLock and Mutex since its simpler.

I will "opt out" and add information to the multithreading issue so we can re-visit it.

std::string baseStoragePath = ModuleSettings::GetStoragePath();
std::string baseStoragePath;
std::map<std::string, std::string>::iterator prop = d->module->coreCtx->frameworkProperties.find("org.osgi.framework.storage");

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Use something like Framework::STORAGE_LOCATION (look up the OSGi standard for a naming suggestion) instead of the raw string literal.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Use something like Framework::STORAGE_LOCATION (look up the OSGi standard for a naming suggestion) instead of the raw string literal.

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 16, 2015

Contributor

This will be fixed as part of this issue

@jeffdiclemente

jeffdiclemente Sep 16, 2015

Contributor

This will be fixed as part of this issue

Show outdated Hide outdated core/src/module/usModulePrivate.h
#include "usAtomicInt_p.h"
#include "usWaitCondition_p.h"

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I think this is not needed (if the inheritance below is remove).

@saschazelzer

saschazelzer Sep 8, 2015

Member

I think this is not needed (if the inheritance below is remove).

Show outdated Hide outdated core/src/module/usModulePrivate.h
@@ -43,7 +44,7 @@ struct ModuleActivator;
/**
* \ingroup MicroServices
*/
class ModulePrivate {
class ModulePrivate : public MultiThreaded<> {

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

I this needed?

@saschazelzer

saschazelzer Sep 8, 2015

Member

I this needed?

Show outdated Hide outdated core/src/util/usFrameworkPrivate_p.h
{
public:
FrameworkPrivate(void);
FrameworkPrivate(std::map<std::string, std::string>& configuration);

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Use const & as parameter type.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Use const & as parameter type.

Show outdated Hide outdated core/src/util/usFrameworkPrivate_p.h
/**
* Make the initialization of the Framework thread-safe.
*/
Mutex* initLock;

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Can we use automatic storage duration instead of a pointer here?

@saschazelzer

saschazelzer Sep 8, 2015

Member

Can we use automatic storage duration instead of a pointer here?

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

yes, I will do that.

@jeffdiclemente

jeffdiclemente Sep 9, 2015

Contributor

yes, I will do that.

Show outdated Hide outdated core/src/util/usFrameworkPrivate_p.h
*/
Mutex* initLock;
CoreModuleContext* coreModuleContext;

This comment has been minimized.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Same as for the Mutex above.

@saschazelzer

saschazelzer Sep 8, 2015

Member

Same as for the Mutex above.

jeffdiclemente added some commits Sep 15, 2015

Changes based on code review
All issues in the code review should be addressed, excluding those that have circle back issues associated with them.

In addition, other issues were fixed:
1. removed more unnecessary #includes.
2. fixed the usCoreExamplesDriver to work with static modules.
3. Removed references in the documentation and CMakeLists.txt files to the US_STATIC_MODULE preprocessor.

Signed-off-by: The MathWorks, Inc. Roy.Lurie@mathworks.com
Fixed bug in Examples unit tests
An incorrect build option was being used in the test.

Signed-off-by: The MathWorks, Inc. Roy.Lurie@mathworks.com
@jeffdiclemente

This comment has been minimized.

Show comment
Hide comment
@jeffdiclemente

jeffdiclemente Sep 21, 2015

Contributor

@saschazelzer, please let me know if there is anything else which requires immediate attention before it can be merged into the development branch. Thanks.

Contributor

jeffdiclemente commented Sep 21, 2015

@saschazelzer, please let me know if there is anything else which requires immediate attention before it can be merged into the development branch. Thanks.

@saschazelzer

This comment has been minimized.

Show comment
Hide comment
@saschazelzer

saschazelzer Sep 22, 2015

Member

Strange, I didn't get any notification about your new changes but maybe I just missed it. I will have a final look and merge it asap. Thanks!

Member

saschazelzer commented Sep 22, 2015

Strange, I didn't get any notification about your new changes but maybe I just missed it. I will have a final look and merge it asap. Thanks!

saschazelzer added a commit that referenced this pull request Sep 22, 2015

@saschazelzer saschazelzer merged commit 6be85bd into CppMicroServices:development Sep 22, 2015

1 of 2 checks passed

continuous-integration/appveyor AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@saschazelzer

This comment has been minimized.

Show comment
Hide comment
@saschazelzer

saschazelzer Sep 22, 2015

Member

All right, thanks a lot for this massive contribution. I will add the discussed todos as new issues.

Member

saschazelzer commented Sep 22, 2015

All right, thanks a lot for this massive contribution. I will add the discussed todos as new issues.

@jeffdiclemente jeffdiclemente deleted the jeffdiclemente:2-remove-static-globals branch Sep 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment