Wrong output for ksc-json-output in case of relative paths under Windows.

[ams-tschoening]

I upgraded ksv to get support for opaque types and with it came a changed interaction with ksc, the new ksc-json-output is used now. This leads to compile time errors in ksv for me, because your generated JSON doesn't seem to handle paths and their different representation in Java under different OS correctly.

C:/[...]/visualizer_main.rb:68:in block (2 levels) in compile_formats': undefined method []' for nil:NilClass (NoMethodError)
from C:/Program Files (x86)/kaitai_struct/visualizer/lib/kaitai/struct/visualizer/visualizer_main.rb:64:in each' from C:/Program Files (x86)/kaitai_struct/visualizer/lib/kaitai/struct/visualizer/visualizer_main.rb:64:in each_with_index'
from C:/Program Files (x86)/kaitai_struct/visualizer/lib/kaitai/struct/visualizer/visualizer_main.rb:64:in `block in compile_formats'

      fns.each_with_index { |fn, idx|
        puts "... processing #{fn} #{idx}"

        log_fn = log[fn]
        if log_fn['errors']

{"record\fmt_clt_recs.ksy": {"firstSpecName": "fmt_clt_recs","output": {"ruby": {"fmt_clt_recs": {"topLevelName": "FmtCltRecs","files": [{"fileName": "fmt_clt_recs.rb"}]},"fmt_oms_rec": {"topLevelName": "FmtOmsRec","files": [{"fileName": "fmt_oms_rec.rb"}]}}}},"record\fmt_oms_rec.ksy": {"firstSpecName": "fmt_oms_rec","output": {"ruby": {"fmt_oms_rec": {"topLevelName": "FmtOmsRec","files": [{"fileName": "fmt_oms_rec.rb"}]}}}}}

There are two problems here:

1. / to \

I use a command line similar to the following in Eclipse to start ksv:

[...]ksv "${workspace_loc}/[...]/data.bin" "record/fmt_clt_recs.ksy" "record/fmt_oms_rec.ksy"

With the current ksc this leads to \ instead of / in the generated JSON object, because Java internally uses a platform dependent path separator most of the times. The problem is that ksv sees / itself and can't find its own paths in the JSON anymore. I used / in favour of \ simply because I had trouble using \ in the past and / seems to be more common in Ruby, this would work on non-Windows etc.

2. Improper escaping of \

\ needs to be escaped in JSON, that's why changing / to \ in my invocation above doesn't fix the problem: JSON contains \f in the end instead of \\f which is what Ruby internally would have as the String key.

In my opinion, the best thing to do is simply to not generate \ in JSON at all, so that only one normalized path separator is used across platforms and languages:

  // FIXME: do proper string handling
  def stringToJson(str: String): String =
    "\"%s\"".format(str.replaceAll("\\\\", "/"))

This is somewhat hacky as well of course, but fixes the escape problem and makes interacting with ksv easier.

[GreyCat]

Thanks for reporting this! I've applied slightly different fix that reuses string output from BaseTranslator, so it should escape quotes, backslashes, etc, properly.

Probably it should be enough for Windows as well. As far as I understand, Ruby on Windows would do fine with backslashes as well. Can't check it, though, my Windows VM are not available at the moment. Could you check if this fix would work for you?

[ams-tschoening]

If I use \ in my relative paths, your change works, with / it doesn't. The problem is that the mentioned invocation with / works on all platforms and is part of some versioned Eclipse launch configuration. \ will break that configuration for my colleagues using Linux.

Don't you think normalizing paths would be worth another hack? JSON itself is OS-independent, so providing such paths as well seems the right thing to do. We do that in some of our software as well for different runtimes and OS like Perl, which automatically convert / to \ sometimes.

[GreyCat]

Ok, understood, I'll add normalization to / in file names then.

[GreyCat]

Should be fixed now: kaitai-io/kaitai_struct_compiler@2aef0de

[ams-tschoening]

Sorry, but I don't think so. Just tested with a current master and the error using / in my relative paths is still the same. Looking at your commit you seemed to have changes some error messages only? What I was talking about was the following line instead.

def stringToJson(str: String): String =
  translator.doStringLiteral(str)

kaitai-io/kaitai_struct_compiler@c94b650#diff-cd926bd86535fd1e7062bc1372c1ec7bR34

That line is providing either / or \ to ksv depending on the current platform.

[GreyCat]

Could you provide an (preferably minimal) example of what is misbehaving? I'll try to find a Windows box to test shortly.

Current stringToJson method seems to do what it is supposed to do: it provides a JSON representation of an arbitrary string. There is absolutely no reason to do any string mangling in such generic method. If we need to replace backslashes in file paths somewhere, then we need to track that particular location and do replacement there.

[ams-tschoening]

Could you provide an (preferably minimal) example of what is misbehaving?

It's not necessarily a misbehaviour, but a design decision. Assume the tests-repo as current working dir, which of the following both invocations should work on Windows and why?

ruby "-I[...]/kaitai_struct/runtime/ruby/lib" "[...]/kaitai_struct/visualizer/bin/ksv" "src/bcd_user_type_be.bin" "formats/bcd_user_type_be.ksy"

ruby "-I[...]/kaitai_struct/runtime/ruby/lib" "[...]/kaitai_struct/visualizer/bin/ksv" "src/bcd_user_type_be.bin" "formats\bcd_user_type_be.ksy"

Currently, the first invocation doesn't work because of formats/, only the second one works. But that second invocation doesn't work on non-Windows.

So the question is, is it worth to add some workaround to JSON output to make the first invocation work on Windows as well? That way the first invocation could be shared amongst platforms easily. E.g. because that invocation is part of some Eclipse launch configuration or some script or whatever.

The reason why the first invocation doesn't work is that ksc provides JSON with \ on Windows always, because Java stringifies paths that way internally, while ksv simply uses what is given on the shell as text. So on Windows one needs to provide \ on the shell, because that is what ends up in JSON, which doesn't work on non-Windows anymore, where Java internally stringifies to /.

If you disagree or are in doubt, just leave it as it is. It's not a huge thing and the most important fix was that of the formal JSON syntax.

I have no other easier examples, my dir layout and such is completely different from yours. But things break down to the both provided invocations, assuming that ksc is properly available in the environment and such.

[generalmimon]

Although the main problem of this issue (invalid string literals like "path\to\format.ksy" in the JSON output) has been resolved in KSC 0.8, there is still the compiler's behavior of converting input paths with / to \ in the --ksc-json-output report on Windows to discuss. As of KSC 0.10, the compiler still converts the path to backslashes even if the input .ksy path uses forward slashes (notice the second test, where the compiler used ".\\hello_world.ksy" as JSON key even though ./hello_world.ksy was given as the input path):

C:\temp\ksc-json-output-tests>kaitai-struct-compiler --ksc-json-output -t ruby -d test-a1 hello_world.ksy
{"hello_world.ksy": {"firstSpecName": "hello_world","output": {"ruby": {"hello_world": {"topLevelName": "HelloWorld","files": [{"fileName": "hello_world.rb"}]}}}}}

C:\temp\ksc-json-output-tests>kaitai-struct-compiler --ksc-json-output -t ruby -d test-a2 ./hello_world.ksy
{".\\hello_world.ksy": {"firstSpecName": "hello_world","output": {"ruby": {"hello_world": {"topLevelName": "HelloWorld","files": [{"fileName": "hello_world.rb"}]}}}}}

C:\temp\ksc-json-output-tests>kaitai-struct-compiler --ksc-json-output -t ruby -d test-a3 .\hello_world.ksy
{".\\hello_world.ksy": {"firstSpecName": "hello_world","output": {"ruby": {"hello_world": {"topLevelName": "HelloWorld","files": [{"fileName": "hello_world.rb"}]}}}}}

Personally, I see this as somewhat unfortunate behavior and don't see any benefit in it. I think reflecting the original .ksy path exactly as passed on the command line to kaitai-struct-compiler (i.e. without any slash conversions or anything else) in the JSON output is the least surprising behavior that would make KSC very easy to use: the naive way of using the input .ksy path directly as a key to the JSON object simply always works regardless of what slashes were used, what platform we're on, etc.

@ams-tschoening:

I use a command line similar to the following in Eclipse to start ksv:

[...]ksv "${workspace_loc}/[...]/data.bin" "record/fmt_clt_recs.ksy" "record/fmt_oms_rec.ksy"

With the current ksc this leads to \ instead of / in the generated JSON object, because Java internally uses a platform dependent path separator most of the times. The problem is that ksv sees / itself and can't find its own paths in the JSON anymore. I used / in favour of \ simply because I had trouble using \ in the past and / seems to be more common in Ruby, this would work on non-Windows etc.

I reported this problem (without knowing about this issue) in kaitai-io/kaitai_struct_visualizer#52 and worked around it in ksv in kaitai-io/kaitai_struct_visualizer@d9a3dbd. Retrying the path with all forward slashes / replaced to backslashes \ seemed to work in simple cases I happened to test, but admittedly, it was a hack without properly investigating the cause of the problem. The simplest proper fix I see would be to just change the compiler to use the input path strings exactly as JSON keys, so that ksv (and possibly other third-party applications making use of --ksc-json-output, if they exist) can reliably just look them up in the JSON object 100% of the time.

[generalmimon]

Apparently, the reason why the compiler converts the input paths is because it initially reads the given command-line .ksy path arguments to java.io.File objects (see JavaMain.scala:38) and later uses their toString() method to convert the File objects back to path strings (JavaMain.scala:279). However, toString() uses the getPath() method:

public String toString()

Returns the pathname string of this abstract pathname. This is just the string returned by the getPath() method.

and getPath() is specified to use the system-dependent separator character:

public String getPath()

Converts this abstract pathname into a pathname string. The resulting string uses the default name-separator character to separate the names in the name sequence.

... which is specified as follows (separatorChar):

public static final char separatorChar

The system-dependent default name-separator character. This field is initialized to contain the first character of the value of the system property file.separator. On UNIX systems the value of this field is '/'; on Microsoft Windows systems it is '\\'.

Also, according to my tests, it collapses multiple consecutive slashes into one (this is in WSL 2, happens on Windows as well):

$ kaitai-struct-compiler -- --ksc-json-output -t ruby -d test-wsl .///hello_world.ksy
{"./hello_world.ksy": {"firstSpecName": "hello_world","output": {"ruby": {"hello_world": {"topLevelName": "HelloWorld","files": [{"fileName": "hello_world.rb"}]}}}}}

Which further proves that merely retrying the path with / replaced by \ as the JSON key that I added in kaitai-io/kaitai_struct_visualizer@d9a3dbd was a band-aid solution, not a proper fix.

[generalmimon]

@ams-tschoening:

Don't you think normalizing paths would be worth another hack? JSON itself is OS-independent, so providing such paths as well seems the right thing to do. We do that in some of our software as well for different runtimes and OS like Perl, which automatically convert / to \ sometimes.

@GreyCat:

Ok, understood, I'll add normalization to / in file names then.

Should be fixed now: kaitai-io/kaitai_struct_compiler@2aef0de

I don't think this "normalization" was the best decision, either. I don't see any compelling reason why it should be done. Also, I'm not sure what it means to provide OS-independent paths - in general, paths are very much OS-dependent. You can achieve some degree of compatibility of relative paths, but not much more.

And the piece of code responsible for this "normalization" is very naive (ProblemCoords.scala:36-38):

object ProblemCoords {
  def formatFileName(file: Option[String]): String = file match {
    case Some(x) => x.replace('\\', '/')

Unix paths use only forward slashes / to separate names, so nothing stops you from creating a file with name hello\world.ksy (not to be confused with a file world.ksy in subdirectory hello - that would be hello/world.ksy). However, this slash normalization code butchers it so that it's indistinguishable from a world.ksy file in a hello subdir:

$ ls -la
total 0
drwxrwxrwx 1 pp pp 4096 Aug 17 11:11  .
drwxrwxrwx 1 pp pp 4096 Aug 17 11:11  ..
-rwxrwxrwx 1 pp pp   68 Aug 17 11:11 'hello\world.ksy'

$ kaitai-struct-compiler -- --ksc-json-output -t ruby -d outdir ./hello\\world.ksy
{"./hello\\world.ksy": {"errors": [{"file": "./hello/world.ksy","path": ["meta","endian"],"message": "unable to parse endianness: `le`, `be` or calculated endianness map is expected"}]}}
                                                    ^

$ kaitai-struct-compiler -- -t ruby -d outdir ./hello\\world.ksy
./hello/world.ksy: /meta/endian:
       ^
	error: unable to parse endianness: `le`, `be` or calculated endianness map is expected

We could spend time tweaking it so it doesn't cause any problems, but as I said, I consider the slash normalization unnecessary. I think it's best when the compiler treats the paths from the user as opaque strings and merely passes them to the underlying platform and all console/JSON outputs unchanged. If the accepted string matches some supported path format on the particular platform, great; if the path is invalid, the platform will report an error, which the compiler just passes along. In fact, there's no need for the compiler to worry about which path formats are supported on which platform.