2:43 a.m.
Hi Patrick,
We've now got the GIO GDBus issues fixed and all the non-autosync &
non-connection tests pass with both DBus wrappers. We are currently
going through the code and making sure code is commented properly,
debugging output is cleaned up and header files are organized more
cleanly. The uncleaned code is in the knowak/for-Chris branch. We
don't suspect to be making anymore logic changes to that, just clean
it up. The clean-up will be finished tomorrow and I'll place it in a
branch named concurrent-sync-sessions-for-review.
Cheers,
Chris
3:53 a.m.
On Tue, 2012-03-06 at 16:43 +0100, Chris Kühl wrote:
We've now got the GIO GDBus issues fixed and all the non-autosync
&
non-connection tests pass with both DBus wrappers. We are currently
going through the code and making sure code is commented properly,
debugging output is cleaned up and header files are organized more
cleanly.
Can you rebase it onto for-master/new-master or even
for-master/platform-modules?
I'm currently running a quick test and will bump master up to
for-master/new-master once that is done.
for-master/platform-modules is a bit more difficult because it includes
some code refactoring which affects the src/dbus/server sources. I
expect conflicts. I don't want to delay your work, but it's likely that
the for-master/platform-modules will go in first.
Perhaps you can have a look whether the changes in
for-master/platform-modules make sense while you are rebasing?
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
2:35 a.m.
On Tue, 2012-03-06 at 17:53 +0100, Patrick Ohly wrote:
On Tue, 2012-03-06 at 16:43 +0100, Chris Kühl wrote:
> We've now got the GIO GDBus issues fixed and all the non-autosync &
> non-connection tests pass with both DBus wrappers. We are currently
> going through the code and making sure code is commented properly,
> debugging output is cleaned up and header files are organized more
> cleanly.
Can you rebase it onto for-master/new-master or even
for-master/platform-modules?
Please ignore for-master/platform-modules for now, it has a regression
visible in test-dbus.py. I'm testing a fix:
diff --git a/src/dbus/server/dbus-sync.cpp b/src/dbus/server/dbus-sync.cpp
index 0c64b9a..4a86c88 100644
--- a/src/dbus/server/dbus-sync.cpp
+++ b/src/dbus/server/dbus-sync.cpp
@@ -26,7 +26,7 @@ SE_BEGIN_CXX
DBusSync::DBusSync(const std::string &config,
Session &session) :
- SyncContext(config),
+ SyncContext(config, true),
m_session(session)
{
setUserInterface(this);
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
2:54 a.m.
On Wed, Mar 7, 2012 at 4:35 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Tue, 2012-03-06 at 17:53 +0100, Patrick Ohly wrote:
> On Tue, 2012-03-06 at 16:43 +0100, Chris Kühl wrote:
> > We've now got the GIO GDBus issues fixed and all the non-autosync &
> > non-connection tests pass with both DBus wrappers. We are currently
> > going through the code and making sure code is commented properly,
> > debugging output is cleaned up and header files are organized more
> > cleanly.
>
> Can you rebase it onto for-master/new-master or even
> for-master/platform-modules?
Please ignore for-master/platform-modules for now, it has a regression
visible in test-dbus.py. I'm testing a fix:
Yeah, we were working on rebasing on to for-master/platform-modules
but were running into issues with the LocalSync and Interactive
password tests. Those tests were passing before we rebased. Taking a
quick look at for-master/platform-modules it seems you are reworking
the password code which would probably break the changes I made to get
interactive passwords working.
Now that you've merged for-master/new-master into master we are
rebasing onto master.
Cheers,
Chris
diff --git a/src/dbus/server/dbus-sync.cpp
b/src/dbus/server/dbus-sync.cpp
index 0c64b9a..4a86c88 100644
--- a/src/dbus/server/dbus-sync.cpp
+++ b/src/dbus/server/dbus-sync.cpp
@@ -26,7 +26,7 @@ SE_BEGIN_CXX
DBusSync::DBusSync(const std::string &config,
Session &session) :
- SyncContext(config),
+ SyncContext(config, true),
m_session(session)
{
setUserInterface(this);
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
2:43 a.m.
On Wed, 2012-03-07 at 16:54 +0100, Chris Kühl wrote:
Now that you've merged for-master/new-master into master we are
rebasing onto master.
How is that going?
I think I fixed all issues on the for-master/platform-modules branch,
but don't let that distract you. Once there is a clean patch series for
current master, I can review and adapt it to the modified password
handling.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
3:18 a.m.
On Thu, Mar 8, 2012 at 4:43 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Wed, 2012-03-07 at 16:54 +0100, Chris Kühl wrote:
> Now that you've merged for-master/new-master into master we are
> rebasing onto master.
How is that going?
It's been rebased and we've squashed the changes into about a dozen
commits. The tests seem to give us the same results as before the
rebase. See the concurrent-sync-sessions-rebased-pre-cleanup branch
for the rebased and squashed changes. I'm putting the final touches on
the cleanup know.
I think I fixed all issues on the for-master/platform-modules
branch,
but don't let that distract you. Once there is a clean patch series for
current master, I can review and adapt it to the modified password
handling.
ok
Cheers,
Chris
3:55 a.m.
On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
On Thu, Mar 8, 2012 at 4:43 PM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Wed, 2012-03-07 at 16:54 +0100, Chris Kühl wrote:
>> Now that you've merged for-master/new-master into master we are
>> rebasing onto master.
>
> How is that going?
>
It's been rebased and we've squashed the changes into about a dozen
commits. The tests seem to give us the same results as before the
rebase. See the concurrent-sync-sessions-rebased-pre-cleanup branch
for the rebased and squashed changes. I'm putting the final touches on
the cleanup know.
057a19f18c600fe4158fd06087aed10b44f20c4e
syncevo: Added methods to add and get environment variables to ForkExec
The parent may need to pass additional environment variables to the
child process.
This commit adds an Add method to the parent process and a Get method
for the child process.
+ /*
+ * Return the value for the environment variable name or and empty
+ * string is not found or set to ForkExecEnvVarEmpty.
+ */
+ std::string getEnvVar(const std::string &name);
Why is this ForkExecEnvVarEmpty necessary?
Note that there is already a GetEnv() in util.h which returns an empty
std::string when getenv() returns NULL.
+std::string ForkExecChild::getEnvVar(const std::string &name)
+{
+ gchar *pValue = getenv(name.c_str());
+ if(!pValue || boost::iequals(pValue, ForkExecEnvVarEmpty)) {
+ return std::string();
+ } else {
+ return std::string(pValue);
+
getenv() returns "char *", not "gchar *". I don't know where that
makes
a difference, but if it is guaranteed to be the same, why introduce
gchar? That implies to me that there might be a difference somewhere.
d85da736dec0975962b610e5c3b0b0dc52b612b3
ForkExec.cpp: Fix the build without --enable-dbus-service.
The .h file's contents are inside an ifdef, so the implementation in
.cpp should be too.
--------------------------- src/syncevo/ForkExec.cpp ---------------------------
index 0395c33..1f71331 100644
@@ -18,8 +18,10 @@
*/
#include "ForkExec.h"
+#if defined(HAVE_GLIB) && defined(DBUS_SERVICE)
+
SE_BEGIN_CXX
#if defined(HAVE_GLIB)
@@ -292,4 +294,5 @@ const char *ForkExecChild::getParentDBusAddress()
#endif // HAVE_GLIB
SE_END_CXX
+#endif // HAVE_GLIB
The if and endif don't match, and HAVE_GLIB is tested twice. I suggest
+#if defined(HAVE_GLIB) && defined(DBUS_SERVICE)
...
-#if defined(HAVE_GLIB)
...
-#endif // HAVE_GLIB
...
+#endif // HAVE_GLIB && DBUS_SERVICE
dd0a4ab2d584004095f5f0e7cd30770ae3d952c8
Add some common DBus callbacks.
These callbacks will be used once sync sessions run in their own
process.
nullCb: a function that does nothing.
counterCb: a function that calls given callback when given counter
drops to zero.
Example use:
...
The documentation should be in comments in the header file, not in the
commit message. That way it is more readily available when reading the
source code in the future.
Note that global functions normally start with a a capital letter, to
distinguish them from member methods, which start with a lower capital.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
4:23 a.m.
On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
On Thu, Mar 8, 2012 at 4:43 PM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Wed, 2012-03-07 at 16:54 +0100, Chris Kühl wrote:
>> Now that you've merged for-master/new-master into master we are
>> rebasing onto master.
>
> How is that going?
>
It's been rebased and we've squashed the changes into about a dozen
commits. The tests seem to give us the same results as before the
rebase. See the concurrent-sync-sessions-rebased-pre-cleanup branch
for the rebased and squashed changes. I'm putting the final touches on
the cleanup know.
6c9a05a9db72f001d9834d2d24ac589f48fc5798
dbus-server: Run sync sessions in separate processes
...
Sessions are separated into SessionResource and Session classes. A
SessionResource instance resides in the server process and serves as a
proxy to the Session instance which is in the child process.
This naming seems rather arbitrary to me. Why call it "Resource" and not
something like "Stub" or "Proxy"?
Or, perhaps even better, don't rename it at all on the server side. Then
a whole range of diffs goes away:
-------------------- src/dbus/server/auto-sync-manager.cpp --------------------
index 53cdceb..a657f0a 100644
@@ -17,10 +17,11 @@
* 02110-1301 USA
*/
#include "auto-sync-manager.h"
-#include "session.h"
+#include "session-resource.h"
#include "server.h"
+#include "dbus-callbacks.h"
...
void AutoSyncManager::startTask()
{
// get the front task and run a sync
// if there has been a session for the front task, do nothing
- if(hasTask() && !m_session) {
+ if(hasTask() && !m_sessionResource) {
But to be fair, I haven't checked whether that makes the diff larger
elsewhere, for example in the actual implementation (session.h/cpp).
Speaking of those, I'd like to see those moved to a separate directory
eventually. There are different coding styles in server (strictly
asynchronous) and in client (may block) and that should be made obvious
by the location of the file.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
4:30 a.m.
On Thu, Mar 8, 2012 at 5:55 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
> On Thu, Mar 8, 2012 at 4:43 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
> > On Wed, 2012-03-07 at 16:54 +0100, Chris Kühl wrote:
> >> Now that you've merged for-master/new-master into master we are
> >> rebasing onto master.
> >
> > How is that going?
> >
>
> It's been rebased and we've squashed the changes into about a dozen
> commits. The tests seem to give us the same results as before the
> rebase. See the concurrent-sync-sessions-rebased-pre-cleanup branch
> for the rebased and squashed changes. I'm putting the final touches on
> the cleanup know.
057a19f18c600fe4158fd06087aed10b44f20c4e
syncevo: Added methods to add and get environment variables to ForkExec
The parent may need to pass additional environment variables to the
child process.
This commit adds an Add method to the parent process and a Get method
for the child process.
+ /*
+ * Return the value for the environment variable name or and empty
+ * string is not found or set to ForkExecEnvVarEmpty.
+ */
+ std::string getEnvVar(const std::string &name);
Why is this ForkExecEnvVarEmpty necessary?
Ah, originally I wanted to use the just the fact that the variable
existed without setting a value but some value needs to be set for env
variables. However, somewhere along the line I changed my mind. I'll
get rid of this.
Note that there is already a GetEnv() in util.h which returns an
empty
std::string when getenv() returns NULL.
Thanks, I wasn't aware. I'll change it to use that.
+std::string ForkExecChild::getEnvVar(const std::string &name)
+{
+ gchar *pValue = getenv(name.c_str());
+ if(!pValue || boost::iequals(pValue, ForkExecEnvVarEmpty)) {
+ return std::string();
+ } else {
+ return std::string(pValue);
+
getenv() returns "char *", not "gchar *". I don't know where that
makes
a difference, but if it is guaranteed to be the same, why introduce
gchar? That implies to me that there might be a difference somewhere.
You're right. Moot point now though.
d85da736dec0975962b610e5c3b0b0dc52b612b3
ForkExec.cpp: Fix the build without --enable-dbus-service.
The .h file's contents are inside an ifdef, so the implementation in
.cpp should be too.
--------------------------- src/syncevo/ForkExec.cpp ---------------------------
index 0395c33..1f71331 100644
@@ -18,8 +18,10 @@
*/
#include "ForkExec.h"
+#if defined(HAVE_GLIB) && defined(DBUS_SERVICE)
+
SE_BEGIN_CXX
#if defined(HAVE_GLIB)
@@ -292,4 +294,5 @@ const char *ForkExecChild::getParentDBusAddress()
#endif // HAVE_GLIB
SE_END_CXX
+#endif // HAVE_GLIB
The if and endif don't match, and HAVE_GLIB is tested twice. I suggest
+#if defined(HAVE_GLIB) && defined(DBUS_SERVICE)
...
-#if defined(HAVE_GLIB)
...
-#endif // HAVE_GLIB
...
+#endif // HAVE_GLIB && DBUS_SERVICE
Yeah, just saw that too.
dd0a4ab2d584004095f5f0e7cd30770ae3d952c8
Add some common DBus callbacks.
These callbacks will be used once sync sessions run in their own
process.
nullCb: a function that does nothing.
counterCb: a function that calls given callback when given counter
drops to zero.
Example use:
...
The documentation should be in comments in the header file, not in the
commit message. That way it is more readily available when reading the
source code in the future.
Note that global functions normally start with a a capital letter, to
distinguish them from member methods, which start with a lower capital.
I'll make these changes and squash them into the for-review branch.
Cheers,
Chris
4:43 a.m.
On Thu, Mar 8, 2012 at 6:30 PM, Chris Kühl <blixtra(a)gmail.com> wrote:
On Thu, Mar 8, 2012 at 5:55 PM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>> On Thu, Mar 8, 2012 at 4:43 PM, Patrick Ohly <patrick.ohly(a)intel.com>
wrote:
>> > On Wed, 2012-03-07 at 16:54 +0100, Chris Kühl wrote:
>> >> Now that you've merged for-master/new-master into master we are
>> >> rebasing onto master.
>> >
>> > How is that going?
>> >
>>
>> It's been rebased and we've squashed the changes into about a dozen
>> commits. The tests seem to give us the same results as before the
>> rebase. See the concurrent-sync-sessions-rebased-pre-cleanup branch
>> for the rebased and squashed changes. I'm putting the final touches on
>> the cleanup know.
>
> 057a19f18c600fe4158fd06087aed10b44f20c4e
>
<snip>
> Note that there is already a GetEnv() in util.h which returns an
empty
> std::string when getenv() returns NULL.
>
Thanks, I wasn't aware. I'll change it to use that.
Hmm, upon inspection I don't actually see this. I only see getEnv()
which returns a char*. I can simply use getenv, though.
Cheers,
Chris
4:58 a.m.
On Thu, Mar 8, 2012 at 6:23 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
> On Thu, Mar 8, 2012 at 4:43 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
> > On Wed, 2012-03-07 at 16:54 +0100, Chris Kühl wrote:
> >> Now that you've merged for-master/new-master into master we are
> >> rebasing onto master.
> >
> > How is that going?
> >
>
> It's been rebased and we've squashed the changes into about a dozen
> commits. The tests seem to give us the same results as before the
> rebase. See the concurrent-sync-sessions-rebased-pre-cleanup branch
> for the rebased and squashed changes. I'm putting the final touches on
> the cleanup know.
6c9a05a9db72f001d9834d2d24ac589f48fc5798
dbus-server: Run sync sessions in separate processes
...
Sessions are separated into SessionResource and Session classes. A
SessionResource instance resides in the server process and serves as a
proxy to the Session instance which is in the child process.
This naming seems rather arbitrary to me. Why call it "Resource" and not
something like "Stub" or "Proxy"?
Yeah, I'm not 100% happy with the naming either. They are subclasses
of Resource so it was the obvious choice. Renaming is not a problem
but I'd rather get finished with the more substantive changes needed
to complete this before doing that.
Or, perhaps even better, don't rename it at all on the server
side. Then
a whole range of diffs goes away:
It's just that the Session is actually not in the Server anymore so it
seems a tad misleading to call it that in the Server.
-------------------- src/dbus/server/auto-sync-manager.cpp
--------------------
index 53cdceb..a657f0a 100644
@@ -17,10 +17,11 @@
* 02110-1301 USA
*/
#include "auto-sync-manager.h"
-#include "session.h"
+#include "session-resource.h"
#include "server.h"
+#include "dbus-callbacks.h"
...
void AutoSyncManager::startTask()
{
// get the front task and run a sync
// if there has been a session for the front task, do nothing
- if(hasTask() && !m_session) {
+ if(hasTask() && !m_sessionResource) {
But to be fair, I haven't checked whether that makes the diff larger
elsewhere, for example in the actual implementation (session.h/cpp).
Speaking of those, I'd like to see those moved to a separate directory
eventually. There are different coding styles in server (strictly
asynchronous) and in client (may block) and that should be made obvious
by the location of the file.
You're speaking of keeping the files that are only used in the helper
in a subdirectory, right?
I'd rather do this towards the end of the process as well? Moving and
renaming files shouldn't be a problem as long as the other pieces are
fine.
Cheers,
Chris
5:13 a.m.
On Thu, 2012-03-08 at 18:43 +0100, Chris Kühl wrote:
>> Note that there is already a GetEnv() in util.h which
returns an empty
>> std::string when getenv() returns NULL.
>>
>
> Thanks, I wasn't aware. I'll change it to use that.
>
Hmm, upon inspection I don't actually see this. I only see getEnv()
which returns a char*.
That's what I meant. It's a bit different from what I remembered; but
std::string(getEnv("foo", "")) achieves the desired effect.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
5:16 a.m.
On Thu, 2012-03-08 at 18:58 +0100, Chris Kühl wrote:
On Thu, Mar 8, 2012 at 6:23 PM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
> 6c9a05a9db72f001d9834d2d24ac589f48fc5798
>
> dbus-server: Run sync sessions in separate processes
>
> ...
>
> Sessions are separated into SessionResource and Session classes. A
> SessionResource instance resides in the server process and serves as a
> proxy to the Session instance which is in the child process.
>
> This naming seems rather arbitrary to me. Why call it "Resource" and not
> something like "Stub" or "Proxy"?
>
Yeah, I'm not 100% happy with the naming either. They are subclasses
of Resource so it was the obvious choice. Renaming is not a problem
but I'd rather get finished with the more substantive changes needed
to complete this before doing that.
Everything that minimizes the number of changes that I need to look at
helps.
> Or, perhaps even better, don't rename it at all on the
server side. Then
> a whole range of diffs goes away:
>
It's just that the Session is actually not in the Server anymore so it
seems a tad misleading to call it that in the Server.
It's still the implementation of the D-Bus Session API in the server,
isn't it?
So for the sake of minimizing code churn, Session (in the server) and
SessionImpl (in the client) might work.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
10:52 a.m.
On Thu, Mar 8, 2012 at 7:16 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Thu, 2012-03-08 at 18:58 +0100, Chris Kühl wrote:
> On Thu, Mar 8, 2012 at 6:23 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
> > On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
> > 6c9a05a9db72f001d9834d2d24ac589f48fc5798
> >
> > dbus-server: Run sync sessions in separate processes
> >
> > ...
> >
> > Sessions are separated into SessionResource and Session classes. A
> > SessionResource instance resides in the server process and serves as a
> > proxy to the Session instance which is in the child process.
> >
> > This naming seems rather arbitrary to me. Why call it "Resource" and
not
> > something like "Stub" or "Proxy"?
> >
>
> Yeah, I'm not 100% happy with the naming either. They are subclasses
> of Resource so it was the obvious choice. Renaming is not a problem
> but I'd rather get finished with the more substantive changes needed
> to complete this before doing that.
Everything that minimizes the number of changes that I need to look at
helps.
It actually increased the changed line count. :(
> > Or, perhaps even better, don't rename it at all on the
server side. Then
> > a whole range of diffs goes away:
> >
>
> It's just that the Session is actually not in the Server anymore so it
> seems a tad misleading to call it that in the Server.
It's still the implementation of the D-Bus Session API in the server,
isn't it?
So for the sake of minimizing code churn, Session (in the server) and
SessionImpl (in the client) might work.
I've gone ahead and done the renaming as well as all the other changes
you've requested except for moving the helper files into a
subdirectory. I've pushed them to a new
concurrent-sync-sessions-for-review branch[1].
As mentioned above renaming has increased the changed line count. I've
got a branch with all the changes minus the renaming if you'd rather
have that.
Cheers,
Chris
[1]
https://meego.gitorious.org/meego-middleware/syncevolution/commits/concur...
6:05 p.m.
On Fri, 2012-03-09 at 00:52 +0100, Chris Kühl wrote:
On Thu, Mar 8, 2012 at 7:16 PM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Thu, 2012-03-08 at 18:58 +0100, Chris Kühl wrote:
>> On Thu, Mar 8, 2012 at 6:23 PM, Patrick Ohly <patrick.ohly(a)intel.com>
wrote:
>> > On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>> > 6c9a05a9db72f001d9834d2d24ac589f48fc5798
>> >
>> > dbus-server: Run sync sessions in separate processes
>> >
>> > ...
>> >
>> > Sessions are separated into SessionResource and Session classes. A
>> > SessionResource instance resides in the server process and serves as a
>> > proxy to the Session instance which is in the child process.
>> >
>> > This naming seems rather arbitrary to me. Why call it "Resource"
and not
>> > something like "Stub" or "Proxy"?
>> >
>>
>> Yeah, I'm not 100% happy with the naming either. They are subclasses
>> of Resource so it was the obvious choice. Renaming is not a problem
>> but I'd rather get finished with the more substantive changes needed
>> to complete this before doing that.
>
> Everything that minimizes the number of changes that I need to look at
> helps.
>
It actually increased the changed line count. :(
I can see how it might do that. But it did reduce the number of chunks
from 152 to 130.
I've gone ahead and done the renaming as well as all the other
changes
you've requested except for moving the helper files into a
subdirectory. I've pushed them to a new
concurrent-sync-sessions-for-review branch[1].
As mentioned above renaming has increased the changed line count. I've
got a branch with all the changes minus the renaming if you'd rather
have that.
Please push it, I'll have a look.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
7:54 p.m.
On Fri, Mar 9, 2012 at 8:05 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Fri, 2012-03-09 at 00:52 +0100, Chris Kühl wrote:
> On Thu, Mar 8, 2012 at 7:16 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
> > On Thu, 2012-03-08 at 18:58 +0100, Chris Kühl wrote:
> >> On Thu, Mar 8, 2012 at 6:23 PM, Patrick Ohly <patrick.ohly(a)intel.com>
wrote:
> >> > On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
> >> > 6c9a05a9db72f001d9834d2d24ac589f48fc5798
> >> >
> >> > dbus-server: Run sync sessions in separate processes
> >> >
> >> > ...
> >> >
> >> > Sessions are separated into SessionResource and Session classes. A
> >> > SessionResource instance resides in the server process and serves as
a
> >> > proxy to the Session instance which is in the child process.
> >> >
> >> > This naming seems rather arbitrary to me. Why call it
"Resource" and not
> >> > something like "Stub" or "Proxy"?
> >> >
> >>
> >> Yeah, I'm not 100% happy with the naming either. They are subclasses
> >> of Resource so it was the obvious choice. Renaming is not a problem
> >> but I'd rather get finished with the more substantive changes needed
> >> to complete this before doing that.
> >
> > Everything that minimizes the number of changes that I need to look at
> > helps.
> >
>
> It actually increased the changed line count. :(
I can see how it might do that. But it did reduce the number of chunks
from 152 to 130.
Yeah, the session and connection files in the helper are quite a bit
larger than those in the server.
> I've gone ahead and done the renaming as well as all the
other changes
> you've requested except for moving the helper files into a
> subdirectory. I've pushed them to a new
> concurrent-sync-sessions-for-review branch[1].
>
> As mentioned above renaming has increased the changed line count. I've
> got a branch with all the changes minus the renaming if you'd rather
> have that.
Please push it, I'll have a look.
Done. It's named concurrent-sync-sessions-for-review-before-rename[1].
Cheers,
chris
[1]
https://meego.gitorious.org/meego-middleware/syncevolution/commits/concur...
6:45 p.m.
On Fri, Mar 9, 2012 at 9:54 AM, Chris Kühl <blixtra(a)gmail.com> wrote:
On Fri, Mar 9, 2012 at 8:05 AM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Fri, 2012-03-09 at 00:52 +0100, Chris Kühl wrote:
>> On Thu, Mar 8, 2012 at 7:16 PM, Patrick Ohly <patrick.ohly(a)intel.com>
wrote:
>> > On Thu, 2012-03-08 at 18:58 +0100, Chris Kühl wrote:
>> >> On Thu, Mar 8, 2012 at 6:23 PM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
>> >> > On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>> >> > 6c9a05a9db72f001d9834d2d24ac589f48fc5798
>> >> >
>> >> > dbus-server: Run sync sessions in separate processes
>> >> >
>> >> > ...
>> >> >
>> >> > Sessions are separated into SessionResource and Session classes.
A
>> >> > SessionResource instance resides in the server process and
serves as a
>> >> > proxy to the Session instance which is in the child process.
>> >> >
>> >> > This naming seems rather arbitrary to me. Why call it
"Resource" and not
>> >> > something like "Stub" or "Proxy"?
>> >> >
>> >>
>> >> Yeah, I'm not 100% happy with the naming either. They are
subclasses
>> >> of Resource so it was the obvious choice. Renaming is not a problem
>> >> but I'd rather get finished with the more substantive changes
needed
>> >> to complete this before doing that.
>> >
>> > Everything that minimizes the number of changes that I need to look at
>> > helps.
>> >
>>
>> It actually increased the changed line count. :(
>
> I can see how it might do that. But it did reduce the number of chunks
> from 152 to 130.
>
Yeah, the session and connection files in the helper are quite a bit
larger than those in the server.
>> I've gone ahead and done the renaming as well as all the other changes
>> you've requested except for moving the helper files into a
>> subdirectory. I've pushed them to a new
>> concurrent-sync-sessions-for-review branch[1].
>>
>> As mentioned above renaming has increased the changed line count. I've
>> got a branch with all the changes minus the renaming if you'd rather
>> have that.
>
> Please push it, I'll have a look.
>
Have you had a chance to look at this?
Cheers,
Chris
Done. It's named concurrent-sync-sessions-for-review-before-rename[1].
Cheers,
chris
[1]
https://meego.gitorious.org/meego-middleware/syncevolution/commits/concur...
8:03 p.m.
On Tue, 2012-03-13 at 08:45 +0100, Chris Kühl wrote:
Have you had a chance to look at this?
I'm ready to include everything except the main patch. Some commits
didn't compile by themselves because they dependent on fixes in later
commits. There were also some other, minor issues. I've cleaned that up
and ran an automated "dist" check with the patch series included:
Branch: for-master/ccs-infrastructure
http://syncev.meego.com/2012-03-12-19-36_dist/lucid-amd64-sources/nightly...
http://syncev.meego.com/2012-03-12-19-36_dist/
Next step is to look at the actual rewrite.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
9:28 p.m.
On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
It's been rebased and we've squashed the changes into about a
dozen
commits. The tests seem to give us the same results as before the
rebase.
And those are? In other words, which tests are known to fail, and why?
I'm running the tests here myself. I'd like to compare notes and avoid
investigating failures that are already known.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
6:42 p.m.
On Wed, Mar 14, 2012 at 11:28 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
> It's been rebased and we've squashed the changes into about a dozen
> commits. The tests seem to give us the same results as before the
> rebase.
And those are? In other words, which tests are known to fail, and why?
I've pushed a branch called css-without-known-test-failures[1] whose
most recent commit comments out all the tests in test/test-dbus.py
that fail. However, there as a bug I introduced during the last
rebase. The fix for that can be found here[2].
All but one of the failures is due to the incomplete AutoSync and
Connection functionality. The restart test has never worked for us on
any branch.
Cheers,
Chris
[1]
https://meego.gitorious.org/meego-middleware/syncevolution/commit/d880525...
[2]
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
7:30 p.m.
2012/3/15 Chris Kühl <blixtra(a)gmail.com>:
On Wed, Mar 14, 2012 at 11:28 AM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>> It's been rebased and we've squashed the changes into about a dozen
>> commits. The tests seem to give us the same results as before the
>> rebase.
>
> And those are? In other words, which tests are known to fail, and why?
>
I've pushed a branch called css-without-known-test-failures[1] whose
most recent commit comments out all the tests in test/test-dbus.py
that fail. However, there as a bug I introduced during the last
rebase. The fix for that can be found here[2].
All but one of the failures is due to the incomplete AutoSync and
Connection functionality. The restart test has never worked for us on
any branch.
That test should work without problems if you run test-dbus.py while
being in the same directory as syncevo-dbus-server binary. That is
because execve's first parameter should be a path (relative or
absolute) to binary and in case of test-dbus.py only
'syncevo-dbus-server' is passed here. It is not absolute path and thus
execlv looks for binary in current running directory only. Seems like
execve does not look for binary in PATH environment variable.
Cheers,
Chris
[1]
https://meego.gitorious.org/meego-middleware/syncevolution/commit/d880525...
[2]
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
_______________________________________________
SyncEvolution mailing list
SyncEvolution(a)syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution
7:32 p.m.
On Thu, 2012-03-15 at 08:42 +0100, Chris Kühl wrote:
On Wed, Mar 14, 2012 at 11:28 AM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>> It's been rebased and we've squashed the changes into about a dozen
>> commits. The tests seem to give us the same results as before the
>> rebase.
>
> And those are? In other words, which tests are known to fail, and why?
>
I've pushed a branch called css-without-known-test-failures[1] whose
most recent commit comments out all the tests in test/test-dbus.py
that fail. However, there as a bug I introduced during the last
rebase. The fix for that can be found here[2].
All but one of the failures is due to the incomplete AutoSync and
Connection functionality.
Incomplete in which sense? Helper-side and internal D-Bus APIs missing?
I can see how that could be a problem for Connection.
But for AutoSync I'm less sure. It's an implementation of a policy,
based on information in syncevo-dbus-server and causing actions there.
What is missing?
The restart test has never worked for us on
any branch.
Hmm, works in the nightly testing.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
7:55 p.m.
On Thu, 2012-03-15 at 09:30 +0100, Krzesimir Nowak wrote:
2012/3/15 Chris Kühl <blixtra(a)gmail.com>:
> On Wed, Mar 14, 2012 at 11:28 AM, Patrick Ohly <patrick.ohly(a)intel.com>
wrote:
>> On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>>> It's been rebased and we've squashed the changes into about a dozen
>>> commits. The tests seem to give us the same results as before the
>>> rebase.
>>
>> And those are? In other words, which tests are known to fail, and why?
>>
>
> I've pushed a branch called css-without-known-test-failures[1] whose
> most recent commit comments out all the tests in test/test-dbus.py
> that fail. However, there as a bug I introduced during the last
> rebase. The fix for that can be found here[2].
>
> All but one of the failures is due to the incomplete AutoSync and
> Connection functionality. The restart test has never worked for us on
> any branch.
That test should work without problems if you run test-dbus.py while
being in the same directory as syncevo-dbus-server binary. That is
because execve's first parameter should be a path (relative or
absolute) to binary and in case of test-dbus.py only
'syncevo-dbus-server' is passed here. It is not absolute path and thus
execlv looks for binary in current running directory only. Seems like
execve does not look for binary in PATH environment variable.
In other words, the initial start of syncevo-dbus-server (via PATH)
works, but restarting it fails (because PATH is not used and the argv[0]
is not found)?
I see how that can happen. The way I run the tests avoids that problem.
It shouldn't be a problem in the main usage of the restart feature
either: when syncevo-dbus-server is activated via D-Bus, it should see a
full path in argv[0].
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
3839
days inactive
3848
days old
syncevolution@synevolution.org
22 comments
3 participants
participants (3)
-
Chris Kühl -
Krzesimir Nowak -
Patrick Ohly