File written with write_file does not end with new line

[jacky8hyf]

https://github.com/bazelbuild/bazel-skylib/blob/505e1bc3aaae8375f857637f78bf6b3dd953862a/rules/private/write_file_private.bzl#L32

write_file uses a simple newline.join(ctx.attr.content), which means the last line does not end with a new line character.

POSIX requires non-empty text files to end with new line characters:

https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/V1_chap03.html

(See 3.185 Line and 3.387 Text File)

So the files written with write_file (which are usually text files, because content is usually a text string list) are usually non-POSIX compliant, unless the user explicitly add an empty token at the end. For example, we do this:

write_file(
    name = "gki_x86_64_protected_modules",
    out = "gki/x86_64/protected_modules",
    content = get_gki_protected_modules_list("x86_64") + [
        "",  # Ensure new line at the end.
    ],
)

But it is easy for developers to forget to do this.

Not being a POSIX file means that other tools may misbehave, e.g. if I cat two files together and the first file doesn't end with the new line, then in the middle, an unexpected line (last line of the first file + first line of the second file) is formed, causing other downstream issues (e.g. a symbol is missing).

I suggest to add a new line to https://github.com/bazelbuild/bazel-skylib/blob/505e1bc3aaae8375f857637f78bf6b3dd953862a/rules/private/write_file_private.bzl#L32 if content is non-empty.

        content = (newline.join(ctx.attr.content) + newline) if ctx.attr.content else "",

But this is a backwards-incompatible change, so I'd rather file an issue to initiate a discussion first. What's the best way to carry this forward, so hacks like the above don't go viral?

[tetromino]

write_file is intended for writing text files, so this is something we ought to fix.

I'd suggest adding a new boolean attribute - say, final_newline - which initially is false by default, and then flipping it to true as an incompatible change (requiring a skylib version bump, and testing in our monorepo to identify any users depending on the newline-less behavior).

[tetromino]

By the way, the "right" default behavior here requires a bit of thought.

• Do we add a newline if content is empty? (An empty file without a newline is perfectly legal, since it has 0 lines.)
• Do we add a newline if the last line of content already ends with a newline?

[jacky8hyf]

By the way, the "right" default behavior here requires a bit of thought.

• Do we add a newline if content is empty? (An empty file without a newline is perfectly legal, since it has 0 lines.)

I don't think we should add a new line if content is empty, as you said.

• Do we add a newline if the last line of content already ends with a newline?

I am not sure about this one, but I am leaning towards "yes". If the last line already ends with a newline, and "final_newline" is true, then it is clearly indicating that there are two new line characters at the end.

That's why I suggested

        content = (newline.join(ctx.attr.content) + newline) if ctx.attr.content else "",

[fmeum]

I am not sure about this one, but I am leaning towards "yes". If the last line already ends with a newline, and "final_newline" is true, then it is clearly indicating that there are two new line characters at the end.

I would find it less surprising if this simply enforced that the content ends with a newline, it adds one all the time. That's similar to what many editors and linters would do and potentially much less breaking when the attribute default is flipped to True.