-
Notifications
You must be signed in to change notification settings - Fork 194
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
Wrong output for ksc-json-output in case of relative paths under Windows. #507
Comments
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? |
If I use 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 |
Ok, understood, I'll add normalization to |
Should be fixed now: kaitai-io/kaitai_struct_compiler@2aef0de |
Sorry, but I don't think so. Just tested with a current master and the error using
kaitai-io/kaitai_struct_compiler@c94b650#diff-cd926bd86535fd1e7062bc1372c1ec7bR34 That line is providing either |
Could you provide an (preferably minimal) example of what is misbehaving? I'll try to find a Windows box to test shortly. Current |
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?
Currently, the first invocation doesn't work because of 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 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 |
Although the main problem of this issue (invalid string literals like
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
I reported this problem (without knowing about this issue) in kaitai-io/kaitai_struct_visualizer#52 and worked around it in |
Apparently, the reason why the compiler converts the input paths is because it initially reads the given command-line .ksy path arguments to
and
... which is specified as follows (
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 |
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 ( object ProblemCoords {
def formatFileName(file: Option[String]): String = file match {
case Some(x) => x.replace('\\', '/') Unix paths use only forward slashes $ 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. |
See kaitai-io/kaitai_struct#507 Before this commit, the input .ksy paths were normalized as soon as they were parsed from command line arguments, because they were read to `java.io.File` objects. [`java.io.File`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/File.html) objects are subjected to path normalization as soon as they're created, for example on Windows the forward slashes `/` are converted to backslashes `\`, a series of consecutive slashes are collapsed into one on both Unix and Windows systems etc. (See kaitai-io/kaitai_struct#507 (comment) for more details.) Consequently, the .ksy file path for which the compile results are reported may not have matched the original input path. This was particularly noticeable on Windows, where you could only use backslashes (not forward slashes) in the .ksy path passed to `ksv` due to this. As I explain in kaitai-io/kaitai_struct#507 (comment), I believe that treating input paths as opaque strings makes the compiler easier to operate. Now you can pass a bunch of .ksy paths to the compiler and expect to find each path string used exactly as a JSON key in `--ksc-json-output`, without any effects of path normalization mentioned earlier.
As demonstrated in kaitai-io/kaitai_struct#507 (comment), this could in fact change the path incorrectly: on a Unix system, a file name can contain `\` as a special character, for example `hello\world.ksy`, but the old code would still "normalize" a Unix path like `./hello\world.ksy` to `./hello/world.ksy`, which is wrong. But I don't see why forced path normalization should be done on Windows either, except for purity reasons (which are unimportant if you ask me). I don't mind any mix of `\` and `/` slashes in the path coming out on Windows (which can indeed happen from now on, especially for paths to imported specs), as long as the resulting path points to the correct file (which it kind of has to, because it is the same path that the compiler used to access the file). This commit reverts 2aef0de.
As explained in kaitai-io/kaitai_struct#507 (comment), retrying the path with all forward slashes `/` replaced with backslashes `\` was a band-aid fix, not a proper solution. It's better to have the compiler preserve input .ksy paths exactly in JSON keys so that applications like `ksv` don't have to do any trial-and-error or complex path resolution. The compiler has already been changed to do that in kaitai-io/kaitai_struct_compiler@4c5096b, so this `ksv` hack can be reverted. This (partially) reverts commit d9a3dbd.
In 2b84e0a, I reverted a previous change that was replacing all backslashes `\` to forward slashes `/` in file paths used in error messages (meaning that backslashes are kept since then if present in input paths). However, until now I didn't notice that this broke `ErrorMessagesSpec` tests on Windows. This is because compile error messages actually include the full path to the `.ksy` spec, for example: ``` ../tests/formats_err/attr_bad_size.ksy: /seq/0/size: error: invalid type: expected integer, got CalcStrType ``` However, in `formats_err` specs, only the base name is specified (see https://github.com/kaitai-io/kaitai_struct_tests/blob/73486eef/formats_err/attr_bad_size.ksy below): ``` # attr_bad_size.ksy: /seq/0/size: # error: invalid type: expected integer, got CalcStrType # meta: id: attr_bad_size seq: # ... ``` The `../tests/formats_err/` prefix in the actual message is supposed to be removed by `.replace(FORMATS_ERR_DIR + "/", "")` in `ErrorMessagesSpec` (line 53). Note that the string to be removed always uses forward slashes `/`. The problem is that on Windows, `f.toString` (which returns a file path with the backslash `\` as name separator - this is explained at kaitai-io/kaitai_struct#507 (comment)) is passed to the compiler as the .ksy spec to process. Since the path normalization (i.e. `\` -> `/` conversion) was removed, error messages will start like `..\tests\formats_err\attr_bad_size.ksy: ...`. This means the `.replace(FORMATS_ERR_DIR + "/", "")` call won't do anything, because it looks for ``../tests/formats_err/`, which is not there. The fix is to pass forward slash `/` paths to the compiler even on Windows.
I believe the proposed solution of "treating paths as opaque strings" is now fully implemented in KSC by commits kaitai-io/kaitai_struct_compiler@7071a9d...2b84e0a. The band-aid commit in This issue is therefore resolved. |
I upgraded
ksv
to get support for opaque types and with it came a changed interaction withksc
, the newksc-json-output
is used now. This leads to compile time errors inksv
for me, because your generated JSON doesn't seem to handle paths and their different representation in Java under different OS correctly.There are two problems here:
I use a command line similar to the following in Eclipse to start
ksv
: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 thatksv
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.\
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:This is somewhat hacky as well of course, but fixes the escape problem and makes interacting with
ksv
easier.The text was updated successfully, but these errors were encountered: