Skip to content
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

Fix the bat #6

Merged
merged 2 commits into from
Feb 21, 2019
Merged

Fix the bat #6

merged 2 commits into from
Feb 21, 2019

Conversation

pwalczysko
Copy link
Member

@pwalczysko pwalczysko commented Jan 25, 2019

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:

  • 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
  • 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
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member

mtbc commented Jan 28, 2019

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. 😃

@mtbc
Copy link
Member

mtbc commented Jan 28, 2019

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
Copy link
Member

mtbc commented Jan 28, 2019

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

@mtbc
Copy link
Member

mtbc commented Jan 28, 2019

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
Copy link
Member

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

@dominikl
Copy link
Member

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

  1. Works, but get a message "Cannot create repository links"
  2. Works, but get a message "Cannot create repository links"
  3. Fails, like expected

👍

@mtbc
Copy link
Member

mtbc commented Jan 28, 2019

"Cannot create repository links":

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

@dominikl
Copy link
Member

Yes, both works @mtbc .

@mtbc
Copy link
Member

mtbc commented Jan 28, 2019

👍 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
Copy link
Member Author

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

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

@pwalczysko
Copy link
Member Author

pwalczysko commented Jan 28, 2019

A bit confused what are the ToDos here.

@mtbc @dominikl ?

@mtbc
Copy link
Member

mtbc commented Jan 29, 2019

Definitely something like #6 (comment) at least but @dominikl may have done that already: i.e. something that needs the quoting.

@mtbc
Copy link
Member

mtbc commented Jan 29, 2019

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
Copy link
Member

mtbc commented Jan 29, 2019

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
Copy link
Member Author

I suppose the test #6 (comment) is covering the task #6 (comment) ?

The last commit should cover the comment #6 (comment)

@pwalczysko
Copy link
Member Author

Maybe merge then ?

@mtbc mtbc merged commit a8422db into ome:master Feb 21, 2019
@pwalczysko pwalczysko deleted the bat branch April 11, 2019 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants