Wrap classpath entries which contains spaces in quotation marks

[hansjoachim]

I work on a project where we've run into problems with the classpath in FitNesse when some of the classpath entries contain spaces.

FitNesse will wrap the classpath in quotation marks when it contains a space, but in our case it fails to read the classpath correctly which cause the test run to fail. The information page after a test execution will display the complete classpath, but indicates that the problem occured at a space and that the rest of the classpath was cut off when it tried to run something. We experimented and found that if we didn't have any entries with spaces, it would run fine, however as soon as we added a jar located in a directory with spaces, it would fail.

We looked into how FitNesse constructs the classpath and found that the current behaviour in ClassPath seems to be that if any of the entries contains a space, the entire classpath is wrapped in quotes. (Additionally the path to the FitNesse-jar is prepended this quoted string in ClientBuilder.buildCommand given that it uses ClientBuilder.DEFAULT_COMMAND_PATTERN, but I don't know whether this affects things or not.) The resulting classpath is then used when running tests.

We did some experimenting by tweaking the classpath and hardcoding it into the test page instead of relying on java.class.path. We found that if we individually wrapped the entries containing spaces, we could run the tests again, so this seems to work better. The attached patch changes the wrapping a bit, from wrapping the whole classpath, to individually wrap those entries containing spaces.

Two comments regarding the patch:

• It could potentially add quotation marks around each element regardless, but I don't know if that makes sense or would just make it look confusing.
• The optional commit additionally removes public access getters which wasn't in use (in FitNesse at least). I guess someone out there might be using it, and I'm not clear on the rules changes in the public API. It's not critical, but I think it is at least a bit odd to have a method returning a list with the underlying elements, when the toString-method will potentially represent them in a different way.

Let me know if you have any questions or comments. :)

PS. This is seems to be the underlying cause of the issue I mentioned in #726, though it appears to have been unrelated.

[amolenaar]

The class path keeps bugging us...

Shouldn't it be enough to quote the class path as a whole? Now we do it only if there is a space involved, but maybe we should just quote it anyway, just as a precaution?

[hansjoachim]

Shouldn't it be enough to quote the class path as a whole?

Maybe. It doesn't seem to work as intended though.

Btw, don't merge this yet. I've done some thinking and realized this patch doesn't do what I attempted to. I'll update the patch and elaborate a bit more soon...

[hansjoachim]

Btw, don't merge this yet. I've done some thinking and realized this patch doesn't do what I attempted to. I'll update the patch and elaborate a bit more soon...

As promised, I'll elaborate a bit. I don't think this approach would have worked as intended and after doing some more research I don't believe this is the real bug after all.

So the problem seemed to be that the classpath was constructed from a mix of paths with and without spaces, for instance "some dir/foo.jar;another dir/bar.jar;third/baz.jar". The idea was that if I could wrap each of the individual elements instead of the combined string, that might work better, like so "some dir/foo.jar";"another dir/bar.jar";third/baz.jar.

After thinking a bit about it and rereading the patch, I realized that my suggestion above would do no such thing, instead it would iterate through the already combined paths and then wrap them. So in essence what the current code is already doing. So I tried with element.split(separator); in order to get access to each individual part. Seemed to run ok, but luckily a test caught why this won't work in practice. Turns out String.split() doesn't take in a String to use when splitting, but a regex expression! Another testcase in FitNesse sent in roughly this fitnesse.jar|foo.jar and by using | as the separator, the result became something like f|i|t|n|e|s|s|e|.|j|a|r|f|o|o|.|j|a|r. That's obviously wrong, and since one cannot really anticipate what people wish to use as separators in the real world, we might run into this at one point. So I don't think this approach will really work at all.

The second reason is that I've done some more debugging and reread #726, and I'm now more convinced that ever that what we run into is the exact same issue. I'll leave another comment there.