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

88 auto install embedded bundles from executable #109

Merged
merged 12 commits into from Aug 1, 2016

Conversation

Projects
None yet
3 participants
@karthikreddy09
Contributor

karthikreddy09 commented Jul 26, 2016

Install all embedded bundles in the executable binary at framework startup.

karthikreddy09 added some commits Jul 26, 2016

Auto-install embedded bundles from the executable
Signed-off-by: The Mathworks Inc. < Roy.Lurie@mathworks.com >
Deleted usBundleUtils.h public header

Signed-off-by: The Mathworks Inc, < Roy.Lurie@mathworks.com>

Removed public header usBundleUtils.h

Signed-off-by: The Mathworks Inc. < Roy.Lurie@mathworks.com >

Fixup platform specific code in BundleUtils & test failure fixes on linux.

Signed-off-by: The Mathworks Inc. < Roy.Lurie@mathworks.com >

Fixes for Mac static builds and windows shared builds.

Signed-off-by: The Mathworks Inc. < Roy.Lurie@mathworks.com>

Auto-install initial changes (works on Mac OS X for shared libs)

Signed-off-by: The Mathworks Inc, < Roy.Lurie@mathworks.com >

Remove symbol lookup for GetBundleContext inline method

Signed-off-by: The Mathworks Inc. <Roy.Lurie@mathworks.com>
Fix a typo
Fixed a wrong commit. 

Signed-off-by: The Mathworks Inc. < Roy.Lurie@mathworks.com >
}
void* GetSymbol_impl(const std::string& bundleName, const std::string& libLocation, const char* symbol)
std::string GetExecutablePath()

This comment has been minimized.

@karthikreddy09

karthikreddy09 Jul 26, 2016

Contributor

Using dlsym to find "main" symbol in the handle returned by dlopen(0, RTLD_LAZY) does not work.
see http://coliru.stacked-crooked.com/a/fd8dd59f2c6c8baa

@karthikreddy09

karthikreddy09 Jul 26, 2016

Contributor

Using dlsym to find "main" symbol in the handle returned by dlopen(0, RTLD_LAZY) does not work.
see http://coliru.stacked-crooked.com/a/fd8dd59f2c6c8baa

This comment has been minimized.

@saschazelzer

saschazelzer Jul 28, 2016

Member

That is because the executable is not linked with -rdynamic, as mentioned in #90 (comment). See also http://coliru.stacked-crooked.com/a/e68dcb2883c24514. I am okay with the current implementation though.

@saschazelzer

saschazelzer Jul 28, 2016

Member

That is because the executable is not linked with -rdynamic, as mentioned in #90 (comment). See also http://coliru.stacked-crooked.com/a/e68dcb2883c24514. I am okay with the current implementation though.

This comment has been minimized.

@karthikreddy09

karthikreddy09 Jul 29, 2016

Contributor

Sorry, I should have read your comment carefully. Clearly, I had my blinders on.

@karthikreddy09

karthikreddy09 Jul 29, 2016

Contributor

Sorry, I should have read your comment carefully. Clearly, I had my blinders on.

karthikreddy09 added some commits Jul 26, 2016

Added a test point for auto-install embedded bundles
Signed-off-by: The Mathworks Inc. < Roy.Lurie@mathworks.com >
Fix test failures
Signed-off-by: The Mathworks Inc. < Roy.Lurie@mathworks.com >
Show outdated Hide outdated core/src/bundle/usBundle.cpp
@@ -138,6 +138,11 @@ void Bundle::Stop(uint32_t options)
void Bundle::Uninstall()
{
if (!IsSharedLibrary(GetLocation()))
{
throw std::runtime_error("Bundles embedded in the executable are not uninstallable");

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Jul 28, 2016

Contributor

The "not uninstallable" part sounds awkward to me. What do you think about something like

Bundles embedded in an executable cannot be uninstalled.

?

@jeffdiclemente

jeffdiclemente Jul 28, 2016

Contributor

The "not uninstallable" part sounds awkward to me. What do you think about something like

Bundles embedded in an executable cannot be uninstalled.

?

Show outdated Hide outdated core/src/bundle/usBundleUtils.cpp
{
handle = GetModuleHandle(nullptr);
US_DEBUG << "GetModuleFileName failed" << GetLastErrorStr();

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Jul 28, 2016

Contributor

all forms of the logger macros US_* are no longer available. Use DIAG_LOG

@jeffdiclemente

jeffdiclemente Jul 28, 2016

Contributor

all forms of the logger macros US_* are no longer available. Use DIAG_LOG

This comment has been minimized.

@karthikreddy09

karthikreddy09 Jul 29, 2016

Contributor

@jeffdiclemente
GetLogSink() is a private method of BundleContext ... I have used std::cerr as the sink. Let me know if that is not the right thing to do.

@karthikreddy09

karthikreddy09 Jul 29, 2016

Contributor

@jeffdiclemente
GetLogSink() is a private method of BundleContext ... I have used std::cerr as the sink. Let me know if that is not the right thing to do.

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Jul 29, 2016

Contributor

you can add these functions as friends in usBundleContext.h. Then use GetBundleContext()

@jeffdiclemente

jeffdiclemente Jul 29, 2016

Contributor

you can add these functions as friends in usBundleContext.h. Then use GetBundleContext()

This comment has been minimized.

@saschazelzer

saschazelzer Jul 29, 2016

Member

Please do not add more friend declarations. This is what GetPrivate is for when used in cpp files. Use GetPrivate(GetBundleContext())->GetLogSink().

@saschazelzer

saschazelzer Jul 29, 2016

Member

Please do not add more friend declarations. This is what GetPrivate is for when used in cpp files. Use GetPrivate(GetBundleContext())->GetLogSink().

This comment has been minimized.

@karthikreddy09

karthikreddy09 Jul 29, 2016

Contributor

Awesome .. Thanks !!

@karthikreddy09

karthikreddy09 Jul 29, 2016

Contributor

Awesome .. Thanks !!

This comment has been minimized.

@karthikreddy09

karthikreddy09 Jul 29, 2016

Contributor

@saschazelzer GetPrivate(GetBundleContext())->GetLogSink() does not work because BundleContextPrivate does not provide a GetLogSink method. It's in BundleContext class.

@karthikreddy09

karthikreddy09 Jul 29, 2016

Contributor

@saschazelzer GetPrivate(GetBundleContext())->GetLogSink() does not work because BundleContextPrivate does not provide a GetLogSink method. It's in BundleContext class.

This comment has been minimized.

@saschazelzer

saschazelzer Jul 29, 2016

Member

Right, my mistake. You should be able to do something like GetPrivate(GetBundleContext())->bundle->coreCtx->sink. We can make the more elegant later.

@saschazelzer

saschazelzer Jul 29, 2016

Member

Right, my mistake. You should be able to do something like GetPrivate(GetBundleContext())->bundle->coreCtx->sink. We can make the more elegant later.

Show outdated Hide outdated core/src/bundle/usBundleRegistry.cpp
if (!allowExecutable && !IsSharedLibrary(location))
{
throw std::runtime_error("Installing bundle at "+ location + " not permitted");

This comment has been minimized.

@saschazelzer

saschazelzer Jul 28, 2016

Member

It would be nice to communicate the real reason why the install failed, e.g.

Cannot install bundles from because it is not a shared library

@saschazelzer

saschazelzer Jul 28, 2016

Member

It would be nice to communicate the real reason why the install failed, e.g.

Cannot install bundles from because it is not a shared library

Show outdated Hide outdated core/src/bundle/usCoreBundleContext.cpp
@@ -120,6 +121,9 @@ void CoreBundleContext::Init()
serviceHooks.Open();
//resolverHooks.Open();
// auto-install all embedded bundles inside the executable
std::vector<Bundle> bundles = bundleRegistry.Install(BundleUtils::GetExecutablePath(), systemBundle.get(), true);

This comment has been minimized.

@saschazelzer

saschazelzer Jul 28, 2016

Member

The bundles variable is never used, it should be removed.

@saschazelzer

saschazelzer Jul 28, 2016

Member

The bundles variable is never used, it should be removed.

Show outdated Hide outdated core/include/usGetBundleContext.h
#define GETBUNDLECONTEXTINSTANCEFCN(Prefix, BundleName) GETBUNDLECONTEXTINSTANCEFCN_(Prefix, BundleName)
#define GETBUNDLECONTEXTINSTANCEFCN_(Prefix, BundleName) Prefix##_##BundleName
extern "C" us::BundleContextPrivate* GETBUNDLECONTEXTINSTANCEFCN(GETBUNDLECONTEXT_FCNPREFIX, US_BUNDLE_NAME)();

This comment has been minimized.

@saschazelzer

saschazelzer Jul 29, 2016

Member

It is a good idea to clean that up and define some common macros. We can do some more cleanup and simplify the GetBundleContext method, see 89dbe3f for my suggestions. If you agree with these changes, you can cherry-pick that commit and apply it to the HEAD of your branch.

@saschazelzer

saschazelzer Jul 29, 2016

Member

It is a good idea to clean that up and define some common macros. We can do some more cleanup and simplify the GetBundleContext method, see 89dbe3f for my suggestions. If you agree with these changes, you can cherry-pick that commit and apply it to the HEAD of your branch.

This comment has been minimized.

@karthikreddy09

karthikreddy09 Jul 29, 2016

Contributor

This is great ... I like the changes. Thanks !! :-)

@karthikreddy09

karthikreddy09 Jul 29, 2016

Contributor

This is great ... I like the changes. Thanks !! :-)

@saschazelzer

This comment has been minimized.

Show comment
Hide comment
@saschazelzer

saschazelzer Jul 29, 2016

Member

This looks good to me for merging, after the comments have been resolved.

Member

saschazelzer commented Jul 29, 2016

This looks good to me for merging, after the comments have been resolved.

saschazelzer and others added some commits Jul 29, 2016

Fix based on review comments
Signed-off-by: The Mathworks Inc. < Roy.Lurie@mathworks.com >
@saschazelzer

This comment has been minimized.

Show comment
Hide comment
@saschazelzer

saschazelzer Jul 29, 2016

Member

Great work. Just to be clear: +1 for merging (the US_DEBUG conflicts could be resolved in the merge commit).

Member

saschazelzer commented Jul 29, 2016

Great work. Just to be clear: +1 for merging (the US_DEBUG conflicts could be resolved in the merge commit).

@karthikreddy09

This comment has been minimized.

Show comment
Hide comment
@karthikreddy09

karthikreddy09 Jul 29, 2016

Contributor

Working on it … :-)
I rebased auto-install onto development and encountered test failures in static builds. Investigating the failures.

-Abhinay.

From: Sascha Zelzer notifications@github.com
Reply-To: CppMicroServices/CppMicroServices reply@reply.github.com
Date: Friday, July 29, 2016 at 1:38 PM
To: CppMicroServices/CppMicroServices CppMicroServices@noreply.github.com
Cc: Abhinay Reddyreddy karthikreddy09@icloud.com, Author author@noreply.github.com
Subject: Re: [CppMicroServices/CppMicroServices] 88 auto install embedded bundles from executable (#109)

Great work. Just to be clear: +1 for merging (the US_DEBUG conflicts could be resolved in the merge commit).


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Contributor

karthikreddy09 commented Jul 29, 2016

Working on it … :-)
I rebased auto-install onto development and encountered test failures in static builds. Investigating the failures.

-Abhinay.

From: Sascha Zelzer notifications@github.com
Reply-To: CppMicroServices/CppMicroServices reply@reply.github.com
Date: Friday, July 29, 2016 at 1:38 PM
To: CppMicroServices/CppMicroServices CppMicroServices@noreply.github.com
Cc: Abhinay Reddyreddy karthikreddy09@icloud.com, Author author@noreply.github.com
Subject: Re: [CppMicroServices/CppMicroServices] 88 auto install embedded bundles from executable (#109)

Great work. Just to be clear: +1 for merging (the US_DEBUG conflicts could be resolved in the merge commit).


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

karthikreddy09 added some commits Jul 29, 2016

Merge remote-tracking branch 'refs/remotes/origin/development'
Conflicts:
	core/include/usBundleUtils.h
	core/src/bundle/usBundleUtils.cpp
	core/test/usFrameworkTest.cpp
Fix for merge issues
Signed-off-by: The Mathworks Inc < Roy.Lurie@mathworks.com >
Use system bundle's LogSink in BundleUtils
Signed-off-by: The Mathworks Inc < Roy.Lurie@mathworks.com >
Show outdated Hide outdated core/src/bundle/usBundleUtils.cpp
// Private util function to return system bundle's log sink
std::shared_ptr<LogSink> GetFrameworkLogSink()
{
auto sink = GetBundleContext().GetLogSink();

This comment has been minimized.

@jeffdiclemente

jeffdiclemente Jul 29, 2016

Contributor

could be simplified to return GetBundleContext().GetLogSink();

@jeffdiclemente

jeffdiclemente Jul 29, 2016

Contributor

could be simplified to return GetBundleContext().GetLogSink();

karthikreddy09 added some commits Jul 29, 2016

Reduced LoC in Util function
Signed-off-by: The Mathworks Inc. < Roy.Lurie@mathworks.com >
Removed friend function introduced in recent commit
Signed-off-by: The Mathworks Inc < Roy.Lurie@mathworks.com >
@jeffdiclemente

This comment has been minimized.

Show comment
Hide comment
@jeffdiclemente

jeffdiclemente Aug 1, 2016

Contributor

This looks good to merge.
Can you please include a change to usBundleEvent.cpp line 108 from

  case BundleEvent::BUNDLE_STOPPED:         return os << "BUNDLE_STOPPED"; 

to

  case BundleEvent::BUNDLE_STOPPED:         return os << "STOPPED"; 

This is a trivial issue which was missed in a previous PR. Thanks.

Contributor

jeffdiclemente commented Aug 1, 2016

This looks good to merge.
Can you please include a change to usBundleEvent.cpp line 108 from

  case BundleEvent::BUNDLE_STOPPED:         return os << "BUNDLE_STOPPED"; 

to

  case BundleEvent::BUNDLE_STOPPED:         return os << "STOPPED"; 

This is a trivial issue which was missed in a previous PR. Thanks.

Fixed ostream output for BundleEvent::Type
Signed-off-by: The Mathworks Inc. < Roy.Lurie@mathworks.com >

@karthikreddy09 karthikreddy09 merged commit 1cd2e24 into development Aug 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jeffdiclemente jeffdiclemente deleted the 88-auto-install-embedded-bundles-from-executable branch Aug 2, 2016

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