Fix the bat - Githubissues

pwalczysko commented 5 years ago

This should suggest a fix of the download.bat script on Windows. The suggested fix is inspired by @joshmoore 's comment pointing to https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroPy/bin/omero.bat

See trello card.

This fix works for me as tested on Windows 7 laptop. Admittedly, I think that I am always testing just the first if case, namely, when my download.bat is in the same dir as the jar. But then, I guess this is the more common case. To be fair, I do not completely understand the second else if with the target inside the path - do not see the target defined in that file.

See what you think

@joshmoore @mtbc

To test:

1.

On Windows, get the downloader from https://github.com/ome/omero-downloader/releases/download/v0.1.2/OMERO.downloader-0.1.2-release.zip
Extract the files and cd into the folder, so that you are in the dir where the download.bat is.
Manually create a target directory, easiest for me was to create a dir directly inside the folder where I was
Run download.bat -b <path-to-directory-you-just-created> -s <server host> -u <user name> -w <password> -f binary Image:<image ID>
you should observe output reporting successful downloads

2.

Create a directory under the dir where your download.bat called "target" and move the downloader-jar-with-dependencies.jar into the directory you just created.
Run again the command from test 1. above
you should observe output reporting successful downloads

- move the downloader-jar-with-dependencies.jar to some other directory than the two dirs described in step 1. and 2. above, or delete it.
- Run again the command from test 1. above
- you should not download anything, instead the output should say Failed to find JAR file named downloader-jar-with-dependencies.jar

joshmoore commented 5 years ago

Looks as I would have thought.

To be fair, I do not completely understand the second else if with the target inside the path - do not see the target defined in that file.

I think this is only the case when you are building the downloader from source.

pwalczysko commented 5 years ago

@joshmoore Added testing steps into description, hope this is as expected (mainly the test 2. where I am trying to mock the structure with the target folder).

mtbc commented 5 years ago

I'm concerned about the case where the %JAR_FILE% needs quoting. Perhaps that's too unlikely though even if it regains version numbering in its name: for some reason what should be easy is not easy enough to be worth it.

Update: Sorry, should have woken up more first, obviously I misread the diff: you can ignore this comment. :smiley:

mtbc commented 5 years ago

For (2) one can just git clone --depth 1 the repo and use the mvn from the test scenario. Doesn't help if your Windows doesn't have devtools installed, admittedly.

mtbc commented 5 years ago

Does the quoting in the if conditions require a similar fix?

mtbc commented 5 years ago

Testing: should also include running download.bat from a directory the path to which actually requires quoting. For example, I can type,

$ foo\ \'\ bar/OMERO.downloader-0.1.2/download.sh  ...

and it works. (I originally downloaded the artifact from within the directory /tmp/foo ' bar/.)

dominikl commented 5 years ago

I suppose I have to replace the bat in the release with the one from the PR right?

dominikl commented 5 years ago

Tested on Windows 10. With the bat from this PR:

Works, but get a message "Cannot create repository links"
Works, but get a message "Cannot create repository links"
Fails, like expected

👍

mtbc commented 5 years ago

"Cannot create repository links":

Does including -l none fix it?
Does running in an administrator console fix it?

dominikl commented 5 years ago

Yes, both works @mtbc .

mtbc commented 5 years ago

:+1: Thank you, it turns out that link-making is an extra special privilege for Windows users! At least we finally have a motivator for the -l option. Noted https://trello.com/c/pXVR5Vyd/56-adapt-to-inability-to-create-symlinks.

pwalczysko commented 5 years ago

Does the quoting in the if conditions require a similar fix?

It works without it, and I do not know why.

pwalczysko commented 5 years ago

A bit confused what are the ToDos here.

@mtbc @dominikl ?

mtbc commented 5 years ago

Definitely something like https://github.com/ome/omero-downloader/pull/6#issuecomment-458046323 at least but @dominikl may have done that already: i.e. something that needs the quoting.

mtbc commented 5 years ago

It works without it, and I do not know why.

Maybe move them anyway to keep it all looking the same? Seems to work either way then readers may be less confused about why there's a difference.

mtbc commented 5 years ago

something that needs the quoting

In my testing with Windows Server 2008 (TS-OME-DEV) using paths with spaces in them, this PR's fix does seem to work.

pwalczysko commented 5 years ago

I suppose the test https://github.com/ome/omero-downloader/pull/6#issuecomment-458540485 is covering the task https://github.com/ome/omero-downloader/pull/6#issuecomment-458452600 ?

The last commit should cover the comment https://github.com/ome/omero-downloader/pull/6#issuecomment-458540051

pwalczysko commented 5 years ago

Maybe merge then ?

ome / omero-downloader

Fix the bat #6