protobuf module: fix truncation issue when unpacking

[jodersky]

It is possible that unpacked files from the classpath share the same name but not the same content. In case an existing unpacked file is replaced by a shorter file, it needs to be properly truncated lest the result be garbage.

A warning messages is logged when a proto file gets overwritten.

Pull request: #2800

[lefou]

Is this even the correct behavior? Having two files with the same path in the classpath is always a call for trouble, as it may fail when the classpath entries have different order. Is this a runtime issue or a compile time issue you try to fix here? It would be nice to have a test case for that, so we make sure, we don't loose it with the next refactoring.

[jodersky]

I agree that having files with the same path on the classpath is most often a sign of trouble, but I would argue that the fix here is correct, because it keeps the same behavior of the current implementation but prevents a certain failure. I don't think that dealing with classpath ordering should be the responsibility of this utility function. Relying on classpath order is dangerous, but we shouldn't introduce artificial hard-failures on it.

The way I came across this error was when cross-building scala JVM and JS versions of a library that needed some common protocol buffer files. Through some transitive dependencies, it turned out that a specific file of the same name was unpacked twice, but once from a version made for Scala JS and once for a version made for Scala JVM. Unfortunately the files did not have the same content, which led to an unreadable final protobuf file because of said bug, leading to a compile error. Now, the best way to deal with this would be to inspect the dependency chain and submit patches to upstream libraries that cause this double-inclusion. However, the classpath order was not strictly the source of the error, and it is possible to prevent the error with my fix.

[lefou]

Yeah, that sounds reasonable. Yet, once we identified a potential source of trouble, it would be nice to make it easier for the developer who runs into it next. Your fix ensures that, disregarding which file we choose, we at least keep it in a valid state. What do you think of an additional check if the file already exists, and print a warning in addition to overwriting the existing file? Easy enough for us, non-disruptive DX as we don't fail, and some hint which helps in case the result isn't the expected one.

[lefou]

Thank you!

[jodersky]

Thanks for the fix! Your rationale makes sense, I agree that a warning here is useful