Combine sy and syd script into one

[binwiederhier]

Currently Syncany has two commands: The sy script to control the regular CLI commands and the syd script to control the daemon. I don't see any reason why these two scripts shouldn't be a single script sy.

To realize this, I suggest using the Gradle-generated script (as per the Gradle application plugin, and the alterations we do in the build.gradle) as a basis and embed the syd script features we developed (start, stop, etc.).

The script would then do the following:

• sy (start|stop|reload|restart|force-stop|status) would be handled by the script itself
• sy (init|connect|up|...) (and all other commands) would be handed over to the Java implementation

Any voluteers?

[vwiencek]

Plus if it is one script, it would be easier to manage different OS (un*x/osx/cygwin)

[vwiencek]

I see two options for merging scripts:

1/ hook into grade application script and do something like

if $1 = "daemon" then \n
//include syncanyd script
else
exec "$JAVACMD" "${JVM_OPTS[@]}" -classpath "$CLASSPATH" org.syncany.cli.Syncany
end if

pros : nice logic
cons : more dependent of future gradle application plugin evolutions

2/ or we could add syncanyd script at the end of the script and force CommandLineClient client to ignore daemon parameter before entering into 'start()' function.
Cons : JVM overhead .... but should not be a problem
Pros : more easy and less prone to bug before we will only have to add syncanyd at the end of the script, directly into startScripts { ... } grade application script

For windows script we could:

• parse script for :execute label
• add the following code juste after this label 'if "%1" == "daemon" goto DAEMON'
• then add syncanyd.bat script at this point

What do you think ?

[binwiederhier]

I don't like (2) at all. Sounds like a dirty hack to me. I'd go with (1) or at least with something similar. I see two options:

Option A)

1. Get rid of the Gradle-provided scripts completely.
2. Add a switch/case a la:

case "$1" in
start)
    start
    ;;
stop)
    stop
    ;;
...
*)
    call_jvm
    ;;

So basically use the current syd script as a basis, and add the "hand over to JVM code" for command not handled by the shell/batch script.

Option B)

1. Keep the Gradle script.
2. Inject the switch/case from option A before the exec "$JAVACMD" ... using the startScripts { stuff.

Option B has the benefit that it will work on more systems, because Gradle takes care of all the weird Darwin/Cygwin stuff...

If there are no objections, I think we should go with B. Would you like to implement this @vwiencek?

[vwiencek]

Ok for me, I'll do it

On Sun, Aug 31, 2014 at 11:34 PM, Philipp C. Heckel <
notifications@github.com> wrote:

I don't like (2) at all. Sounds like a dirty hack to me. I'd go with (1)
or at least with something similar. I see two options:

Option A)

1. Get rid of the Gradle-provided scripts completely.
2. Add a switch/case a la:

case "$1" in
start)
start
;;
stop)
stop
;;
...
*)
call_jvm
;;

So basically use the current syd script as a basis, and add the "hand
over to JVM code" for command not handled by the shell/batch script.

Option B)

1. Keep the Gradle script.
2. Inject the switch/case from option A before the exec "$JAVACMD" ...
   using the startScripts { stuff.

Option B has the benefit that it will work on more systems, because Gradle
takes care of all the weird Darwin/Cygwin stuff...

If there are no objections, I think we should go with B. Would you like to
implement this @vwiencek https://github.com/vwiencek?

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

[binwiederhier]

Can you maybe make a new branch for that and merge lib/daemon in it? It's probably easier to have that in one step. Relates to #215.

[vwiencek]

Ok

some thoughts
==> syncany-message depends on syncany-lib ==> in order to avoid cycle
dependencies, we should integrate syncany-messages into lib
==> syncany-daemon depends on syncany-cli ... thus lib should depend on cli
which is not wishable....

trying to figure out a way to do this.

On Mon, Sep 1, 2014 at 10:55 AM, Philipp C. Heckel <notifications@github.com

wrote:

Can you maybe make a new branch for that and merge lib/daemon in it? It's
probably easier to have that in one step. Relates to #215
#215.

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

[binwiederhier]

Okay this is getting ugly. We definitely don't want this.

But afaik syncany-daemon only depends on syncany-cli because of the DaemonCommand. This would move over to syncany-cli.

gradle dependencies output (shortened):

compile - Compile classpath for source set 'main'.
+--- project :syncany-lib
|    +--- project :syncany-util
+--- project :syncany-cli
|    +--- project :syncany-lib (*)
|    +--- project :syncany-util (*)
|    +--- project :syncany-messages
|    |    +--- project :syncany-lib (*)
|    |    \--- project :syncany-util (*)
+--- project :syncany-util (*)
+--- project :syncany-daemon
|    +--- project :syncany-lib (*)
|    +--- project :syncany-util (*)
|    +--- project :syncany-cli (*)
|    +--- project :syncany-messages (*)
\--- project :syncany-messages (*)

[vwiencek]

Hi all

you can find my proposal for daemon/lib merger && script merging
https://github.com/vwiencek/syncany/tree/fix-script

As for now
--> daemon & messages have been merged to lib
--> unix script sy|syncany now handles daemon : I kept syncanyd script into
gradle/daemon folder, ans I merge it when start script is build with gradle
application plugin
--> windows script : my god WHO DESIGNED BATCH LANGUAGE ==> 3 hours lost so
far for making in work when included in sy.bat script ==> workaround is to
"call" syncanyd script .... that means that for now we still have to keep
syncanyd script into bin folder....

Vincent

On Mon, Sep 1, 2014 at 4:30 PM, Philipp C. Heckel notifications@github.com
wrote:

Okay this is getting ugly. We definitely don't want this.

But afaik syncany-daemon only depends on syncany-cli because of the
DaemonCommand. This would move over to syncany-cli.

gradle dependencies output (shortened):

compile - Compile classpath for source set 'main'.
+--- project :syncany-lib
| +--- project :syncany-util
+--- project :syncany-cli
| +--- project :syncany-lib ()
| +--- project :syncany-util ()
| +--- project :syncany-messages
| | +--- project :syncany-lib ()
| | --- project :syncany-util ()
+--- project :syncany-util ()
+--- project :syncany-daemon
| +--- project :syncany-lib ()
| +--- project :syncany-util ()
| +--- project :syncany-cli ()
| +--- project :syncany-messages ()
--- project :syncany-messages ()

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

[binwiederhier]

Looks good so far. A few remarks:

1. I am not entirely happy with the syncany-messages module being included in the lib. That means the GUI will have to depend on the entire syncany-lib...
2. Can you remove the .gitignore file again https://github.com/vwiencek/syncany/compare/fix-script#diff-f2f65936cb86ba6961a9d86e76090be0R1? That seems to happen with your commits? Do you use some software that adds that automatically?
3. Remove commenting: https://github.com/vwiencek/syncany/compare/fix-script#diff-5b61f84542454804843085b01347b2c1R14
4. Remove commenting: https://github.com/vwiencek/syncany/compare/fix-script#diff-4d090ebb1b3b769c70ac99bc0dd5e45bR244 Why did you add that?
5. Why did you remove the echo -n? The output now breaks: "Starting syncany\n.......".
6. The Linux shell script's start() function calls syncany daemon (recursion!). We should call the Java process directly now. I think we don't actually need get_realpath anymore. I know we just added it ...
7. I actually thought we'd go with sy (start|stop|status|...), but I see how sy status conflicts with the regular command, so I think your appreach with sy daemon (start|stop|...) is good.
8. The switch/case in the Linux script is missing some cases.
9. What is the export PATH="${PATH:+$PATH:}/usr/sbin:/sbin" in the script for? Do we need anything from sbin? Isn't sbin included by default?
10. The resulting Linux script looks wild. As soon as everything is working, we'll need to do some cleanup: Grouping of constants/variables, line breaks, those kind of things.
11. I feel you with regards to BATCH files. It took me days (!) to get the current daemon stuff working as my wonderful comment (DO NOT TOUCH THIS CODE!) indicates.

[vwiencek]

Ok for all except 1, because right now syncing-messages depends on
syncing-lib ; indeed, Message implementation often refer to core/lib
objects (FileVersion ....)
So two options

• merge messages into lib ==> but GUI will have to depdend on lib
• decouple message from lib by reimplementing core objects : FileHistoryId,
  FileChecksum, FileVersion, FileType and DatabaseVersionHeader

Vincent

On Mon, Sep 1, 2014 at 10:43 PM, Philipp C. Heckel <notifications@github.com

wrote:

Looks good so far. A few remarks:

1. I am not entirely happy with the syncany-messages module being
   included in the lib. That means the GUI will have to depend on the entire
   syncany-lib...
2. Can you remove the .gitignore file again
   https://github.com/vwiencek/syncany/compare/fix-script#diff-f2f65936cb86ba6961a9d86e76090be0R1?
   That seems to happen with your commits? Do you use some software that adds
   that automatically?
3. Remove commenting:
   https://github.com/vwiencek/syncany/compare/fix-script#diff-5b61f84542454804843085b01347b2c1R14
4. Remove commenting:
   https://github.com/vwiencek/syncany/compare/fix-script#diff-4d090ebb1b3b769c70ac99bc0dd5e45bR244
   Why did you add that?
5. Why did you remove the echo -n? The output now breaks: "Starting
   syncany\n.......".
6. The Linux shell script's start() function calls syncany daemon
   (recursion!). We should call the Java process directly now. I think we
   don't actually need get_realpath anymore. I know we just added it ...
7. I actually thought we'd go with sy (start|stop|status|...), but I
   see how sy status conflicts with the regular command, so I think your
   appreach with sy daemon (start|stop|...) is good.
8. The switch/case in the Linux script is missing some cases.
9. What is the export PATH="${PATH:+$PATH:}/usr/sbin:/sbin" in the
   script for? Do we need anything from sbin? Isn't sbin included by default?
10. The resulting Linux script looks wild. As soon as everything is
    working, we'll need to do some cleanup: Grouping of constants/variables,
    line breaks, those kind of things.
11. I feel you with regards to BATCH files. It took me days (!) to get
    the current daemon stuff working as my wonderful comment (DO NOT
    TOUCH THIS CODE!) indicates.

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

[binwiederhier]

Leave it like it is. I don't see a good way except to merge the messages module as well.

[vwiencek]

You prefer to keep message / daemon / lib separated ? or merge all ?

On Tue, Sep 2, 2014 at 2:09 PM, Philipp C. Heckel notifications@github.com
wrote:

Leave it like it is. I don't see a good way except to merge the messages
module as well.

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

[binwiederhier]

Merged. ;-)

[vwiencek]

ok .... best way to decouple message from lib would be to have POJO
messages only using basic types .... but you loose type check ...
there is no free lunch

On Tue, Sep 2, 2014 at 2:44 PM, Philipp C. Heckel notifications@github.com
wrote:

Merged. ;-)

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

[binwiederhier]

I rewrote the Linux script a bit to make it nicer, see branch https://github.com/syncany/syncany/tree/feature/scripts

Also:
(12) Can you try to use coding guidelines and the branching model (name feature branches feature/).
(13) Try to look at the git-compare output to see if there are unusal things, like a whitespace that you didn't expect: https://github.com/syncany/syncany/compare/feature/scripts?expand=1#diff-4d090ebb1b3b769c70ac99bc0dd5e45bR2
(14) Can you answer (9)?
(15) I just realized that we now cannot run CLI commands via the REST API anymore. This is not cool. We have to find a solution for that ...

[vwiencek]

Ok I'll look at that I think the PATH stuff was there before ... 12/ok 13/ok 14/how did you realise that ... That I can try to reproduce it !

---Sent from Boxer | http://getboxer.com

On 2 septembre 2014 19:24:12 UTC+2, Philipp C. Heckel notifications@github.com wrote:I rewrote the Linux script a bit to make it nicer, see branch https://github.com/syncany/syncany/tree/feature/scripts Also: (12) Can you try to use coding guidelines and the branching model (name feature branches feature/ ). (13) Try to look at the git-compare output to see if there are unusal things, like a whitespace that you didn't expect: https://github.com/syncany/syncany/compare/feature/scripts?expand=1#diff-4d090ebb1b3b769c70ac99bc0dd5e45bR2 (14) Can you answer (9)? (15) I just realized that we now cannot run CLI commands via the REST API anymore. This is not cool. We have to find a solution for that ... —Reply to this email directly or view it on GitHub.

[binwiederhier]

The code you commented in the WatchRunner executes incoming CliRequests. To reproduce launch the daemon with one daemon-managed Syncany folder. Then navigate to that folder via the command line and run sy status. The CLI realizes that the folder is daemon managed and sends a CliRequest to the daemon.

[binwiederhier]

@ all
So (15) really bugs me. It removes functionality that was there before. However, it's not entirely clear to me if the approach we took here was the right to begin with:

a. Right now, if a folder is Syncany-managed, the CLI finds a port.xml file in .syncany and sends the command line parameters via REST in a CliRequest (<CliRequest><args>restore --revision=123...</args></CliRequest>) to the daemon. The request is then processed by the daemon. For some commands this doesn't make sense (sy init, ...).

b. Another possibility would be to parse the CLI params in the CLI and make useful REST commands out of the ones that actually make sense (restore, up, down, status, ..., ls). This is probably the better way to do it, but it requires a lot of extra work and manual mapping between CLI command and REST request.

Any thought? (was that understandable?)

@vwiencek With the exception of (15) and the windows batch script, everything is fine now. Can you combine the batch script? Or do you want me to do that?

[binwiederhier]

Code for (a):
The CommandLineClient send the request, the WatchRunner receive it.

[vwiencek]

I'm confused because there is still a dependency issue.
@binwiederhier https://github.com/binwiederhier your feature/scripts
branch is not working because I commented handleCliRequestNoSyncRunning in
WatchRunner.

Indeed this method rely on Command object which is part of syncing-cli
package.

I think we should get rid of *Command objects and use instead *Operations
.. meaning we have to populate *Operations objects (kind of what is done in
*Command objects .....).

Your proposition would be to call directly correct commands inside CLI and
get rid of CliRequest (which is generic) ?

https://github.com/syncany/syncany/blob/feature/scripts/syncany-lib/src/main/java/org/syncany/operations/daemon/WatchRunner.java#L243

On Wed, Sep 3, 2014 at 8:14 AM, Philipp C. Heckel notifications@github.com
wrote:

@ all
So (15) really bugs me. It removes functionality that was there before.
However, it's not entirely clear to me if the approach we took here was the
right to begin with:

Right now, if a folder is Syncany-managed, the CLI finds a port.xml
file in .syncany and sends the command line parameters via REST in a
CliRequest (restore
--revision=123...) to the daemon. The request is
then processed by the daemon. For some commands this doesn't make sense (sy
init, ...).
-

Another possibility would be to parse the CLI params in the CLI and
make useful REST commands out of the ones that actually make sense
(restore, up, down, status, ..., ls). This is probably the better way to do
it, but it requires a lot of extra work and manual mapping between CLI
command and REST request.

Any thought? (was that understandable?)

@vwiencek https://github.com/vwiencek With the exception of (15) and
the windows batch script, everything is fine now. Can you combine the batch
script? Or do you want me to do that?

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

[vwiencek]

Ok I got it.
So I propose the following:

• integrate printResults() method inside *OperationResult object ;
• create *Operation objects inside CLI and send them via rest ....

what do you think ?

On Wed, Sep 3, 2014 at 9:06 AM, Vincent Wiencek vwiencek@gmail.com wrote:

I'm confused because there is still a dependency issue.
@binwiederhier https://github.com/binwiederhier your feature/scripts
branch is not working because I commented handleCliRequestNoSyncRunning in
WatchRunner.

Indeed this method rely on Command object which is part of syncing-cli
package.

I think we should get rid of *Command objects and use instead *Operations
.. meaning we have to populate *Operations objects (kind of what is done in
*Command objects .....).

Your proposition would be to call directly correct commands inside CLI and
get rid of CliRequest (which is generic) ?

https://github.com/syncany/syncany/blob/feature/scripts/syncany-lib/src/main/java/org/syncany/operations/daemon/WatchRunner.java#L243

On Wed, Sep 3, 2014 at 8:14 AM, Philipp C. Heckel <
notifications@github.com> wrote:

@ all
So (15) really bugs me. It removes functionality that was there before.
However, it's not entirely clear to me if the approach we took here was the
right to begin with:

Right now, if a folder is Syncany-managed, the CLI finds a port.xml
file in .syncany and sends the command line parameters via REST in a
CliRequest (restore
--revision=123...) to the daemon. The request is
then processed by the daemon. For some commands this doesn't make sense (sy
init, ...).
-

Another possibility would be to parse the CLI params in the CLI and
make useful REST commands out of the ones that actually make sense
(restore, up, down, status, ..., ls). This is probably the better way to do
it, but it requires a lot of extra work and manual mapping between CLI
command and REST request.

Any thought? (was that understandable?)

@vwiencek https://github.com/vwiencek With the exception of (15) and
the windows batch script, everything is fine now. Can you combine the batch
script? Or do you want me to do that?

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

Vincent Wiencek
vwiencek@gmail.com

[binwiederhier]

I strongly object moving the printResults() method to the operation result -- since the operations are presentation-agnostic (meaning they don't care how results are presented to the consumer; CLI output looks different to GUI output, etc.).

I'd rather create use-case specific REST requests for the operations that actually make sense.

Example 1:

1. User calls sy status
2. CommandLineClient creates StatusRequest which contains StatusOperationOptions (does not yet exist) and sends it via REST

Example 2:

1. User calls sy restore
2. CommandLineClient creates RestoreRequest which contains RestoreOperationOptions (already exists, but does not contain operation options) and sends it via REST

And so on ...

The question is: Can we do that for all requests/operations transparently (= magically map "BlaRequest" to "BlaOperation"?!

[vwiencek]

ok that's what I was doing ... except for the printresult :)

Vincent

On Wed, Sep 3, 2014 at 11:36 AM, Philipp C. Heckel <notifications@github.com

wrote:

I strongly object moving the printResults() method to the operation result
-- since the operations are presentation-agnostic (meaning they don't care
how results are presented to the consumer; CLI output looks different to
GUI output, etc.).

I'd rather create use-case specific REST requests for the operations that
actually make sense.

Example 1:

1. User calls sy status
2. CommandLineClient creates StatusRequest which contains
   StatusOperationOptions (does not yet exist) and sends it via REST

Example 2:

1. User calls sy restore
2. CommandLineClient creates RestoreRequest which contains
   RestoreOperationOptions (already exists, but does not contain operation
   options) and sends it via REST

And so on ...

The question is: Can we do that for all requests/operations transparently
(= magically map "BlaRequest" to "BlaOperation"?!

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

[binwiederhier]

Don't do too much in one branch. It makes it impossible to merge it if you keep developing in one branch. I'd leave this out of the feature/script branch and focus on the .bat file for this branch.

You can base another branch on feature/script to do the CliRequest stuff.

[vwiencek]

Chat do you mean by "combine the batch script"?

Envoyé de mon iPhone

Le 3 sept. 2014 à 08:14, Philipp C. Heckel notifications@github.com a écrit :

@ all
So (15) really bugs me. It removes functionality that was there before. However, it's not entirely clear to me if the approach we took here was the right to begin with:

Right now, if a folder is Syncany-managed, the CLI finds a port.xml file in .syncany and sends the command line parameters via REST in a CliRequest (restore --revision=123...) to the daemon. The request is then processed by the daemon. For some commands this doesn't make sense (sy init, ...).

Another possibility would be to parse the CLI params in the CLI and make useful REST commands out of the ones that actually make sense (restore, up, down, status, ..., ls). This is probably the better way to do it, but it requires a lot of extra work and manual mapping between CLI command and REST request.

Any thought? (was that understandable?)

@vwiencek With the exception of (15) and the windows batch script, everything is fine now. Can you combine the batch script? Or do you want me to do that?

—
Reply to this email directly or view it on GitHub.

[binwiederhier]

I'd like a combined syd.bat and sy.bat in the new sy.bat. Calling syd.bat from sy.bat is not enough. See above:

"@vwiencek With the exception of (15) and the windows batch script, everything is fine now. Can you combine the batch script? Or do you want me to do that?"

[vwiencek]

ok i'll do it tonight once I got my windows machine

On Wed, Sep 3, 2014 at 11:50 AM, Philipp C. Heckel <notifications@github.com

wrote:

I'd like a combined syd.bat and sy.bat in the new sy.bat. Calling syd.bat
from sy.bat is not enough. See above:

"@vwiencek https://github.com/vwiencek With the exception of (15) and
the windows batch script, everything is fine now. Can you combine the batch
script? Or do you want me to do that?"

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

[binwiederhier]

I know this is probably not your favorite task, but can you do the batch script stuff first and we'll open another issue for the CLI-to-daemon communication?

The reason is that we can close #215, #211, #210 (this issue) and #153 after that -- because they all rely on the merging of the modules.

[vwiencek]

yes I'm on it
:)
vincent

On Wed, Sep 3, 2014 at 10:00 PM, Philipp C. Heckel <notifications@github.com

wrote:

I know this is probably not your favorite task, but can you do the batch
script stuff first and we'll open another issue for the CLI-to-daemon
communication?

The reason is that we can close #215
#215, #211
#211, #210
#210 (this issue) and #153
#153 after that -- because
they all rely on the merging of the modules.

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

[binwiederhier]

Ah cool. Thanks!

[vwiencek]

done

FYI : if condition ( command ) else ( ) suc*s because it leads to some
weird variable initialisation issues ..... please prefer das wunderbar
goto/label
:)

On Wed, Sep 3, 2014 at 10:35 PM, Philipp C. Heckel <notifications@github.com

wrote:

Ah cool. Thanks!

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

[binwiederhier]

Looks very good and clean to me. See vwiencek@4adb609.

I'll test it tonight with my Windows VM and if it works properly, we can merge it and close/merge the above mentioned issues.

[vwiencek]

great

On Thu, Sep 4, 2014 at 3:04 PM, Philipp C. Heckel notifications@github.com
wrote:

Looks very good and clean to me.
vwiencek@4adb609?diff=split

I'll test it tonight with my Windows VM and if it works properly, we can
merge it and close/merge the above mentioned issues
#210 (comment).

—
Reply to this email directly or view it on GitHub
#210 (comment).

Vincent Wiencek
vwiencek@gmail.com

[binwiederhier]

Merged in ddba59a. Now lets see how Travis reacts with the tests. Maybe I should have created a pull request first ...

If Travis is happy, we'll close this (and the other issues).

[binwiederhier]

Travis was a good boy. https://travis-ci.org/syncany/syncany/builds/34412004
Hereby closed. Yeyy.