google / go-jsonnet

Apache License 2.0
1.63k stars 235 forks source link

Make trailing newline behavior consistent with c++ version #518

Open discordianfish opened 3 years ago

discordianfish commented 3 years ago

Hi,

it looks like the c++ version adds a newline to the end of the output (at least withj the --multi dir option) but the golang version does not. According to POSIX, a file should end with a newline so I'd suggest changing the go version to also include the newline:

Here is a diff for a file generated by c++, then re-generated by the go version:

--- a/tests/expected/deployment.yaml
+++ b/tests/expected/deployment.yaml
@@ -39,4 +39,4 @@
       "volumes":
       - "hostPath":
           "path": "/data/minecraft"
-        "name": "data"
+        "name": "data"
\ No newline at end of file
sbarzowski commented 3 years ago

Hmmm... How exactly is this produced? Is C++ version producing the newlines with -S when the final string doesn't end in a newline? I'm assuming that -S is used since it's yaml, so it needed to be serialized somehow.

discordianfish commented 3 years ago

Sorry if my report wasn't clear, it doesn't depend on yaml. Looks like it happens with any string:

$ ./go/jsonnet --version
Jsonnet commandline interpreter v0.17.0
$ ./cpp/jsonnet --version
Jsonnet commandline interpreter v0.17.0
$ echo '{ "foo": "x" }' | ./go/jsonnet -S -m . - && cat foo  |xxd
./foo
00000000: 78                                       x
$ echo '{ "foo": "x" }' | ./cpp/jsonnet -S -m . - && cat foo  |xxd
./foo
00000000: 780a                                     x.

Also doesn't seem to depend on whether there is already a newline or not. The c++ version seem to just add a newline:

$ echo '{ "foo": "x\n" }' | ./cpp/jsonnet -S -m . - && cat foo  |xxd
./foo
00000000: 780a 0a                                  x..
$ echo '{ "foo": "x\n" }' | ./go/jsonnet -S -m . - && cat foo  |xxd
./foo
00000000: 780a                                     x.

No difference when not using -m though:

$ echo '{ "foo": "x" }.foo' | ./go/jsonnet  -S -
x
$ echo '{ "foo": "x" }.foo' | ./cpp/jsonnet  -S -
sm-gravid-day commented 3 years ago

Bump - this is very annoying as it means you can't seamlessly switch between implementations without your version control system detecting changes.

sbarzowski commented 3 years ago

Here it is in cpp-jsonnet: https://github.com/google/jsonnet/blob/a8769d48f96e0d419cd18190f16c294cce134f4d/core/libjsonnet.cpp#L548.

I'm leaning towards fixing it on cpp side (removing the extra newline). The go-jsonnet behavior seems more reasonable here (and it's not mandated by the semantics, this is just a presentation thing). You might sometimes want to have output files without trailing newlines (e.g. if you produce binary files).

Perhaps we'll need to distinguish raw string output (-S) vs regular output and still add the newline in the latter case.

@sparkprime Thoughts?

sm-gravid-day commented 3 years ago

I agree with @discordianfish that the go version should be changed to add a newline for POSIX compliance.

sbarzowski commented 3 years ago

Hmmm... it's UTF-8 anyway, so we don't really allow arbitrary binary data in this context (which is a shame btw, and I hope to fix that one day). So, it's technically always a text file, so a newline makes sense.

sbarzowski commented 3 years ago

If someone feels like sending a PR, that would be great. Otherwise I will fix it myself, but it will take longer as there's a bunch of other things on my list.