include's in hierarchic proto definitions use static path separator

[asprionj-NO]

import statements in the .proto files lead to include statements in the generated julia code.
It seems that currently, OS-specific path separators are hard-coded in the generated code. At least I see \\ under Windows, and / under Linux. This makes code generated under Windows unusable under Linux.

• Using / as well under Windows would work.
• Cleanest would be to use joinpath.

Is there any reason not to do it, or potential pitfalls? I could not directly figure out where the exact paths are constructed and handed around, to then end up in the generated files. If you point me to it, I can also make a PR.

[asprionj-NO]

Currently, repr is used to generate string values that reproduce the string file-paths obtained by joinpath or abspath, when printing the include statements in codegen/modules.jl on lines 168, 172 and 179.

Lines 168 and 179 could directly print a joinpath statement. Current code (example is line 168, 179 similar):

println(io, "include(", repr(joinpath(submodule_name, string(submodule_name, ".jl"))), ")")

For submodule_name = "Sub", this prints (on Windows):

include("Sub\\Sub.jl")

Could change this to:

println(io, """include(joinpath("$submodule_name", "$submodule_name.jl"))""")

Which would print:

include(joinpath("Sub", "Sub.jl"))

For line 172, we'd have first split the path, then use a joinpath statement in the generated code, to join all parts of the path again. Current version:

println(io, "include(", repr(proto_script_name(file)), ")")

Example (on Windows), assuming script_file = proto_script_name(file) = "C:\\Users\\JohnDoe\\SomePath\\SomeProtoFolder\\External.proto", this produces:

include("C:\\Users\\JohnDoe\\SomePath\\SomeProtoFolder\\External.proto")

This could be changed to:

println(io, reduce((str, spp) -> str*"\"$spp\",", splitpath(script_name), init="joinpath(")[1:end-1]*")")

to produce:

joinpath("C:\","Users","JohnDoe","SomePath","SomeProtoFolder","External.proto")

Similar changes would have to be done for lines 115 and 122, for the external packages and non-package dependencies, respectively.

The same for codegen/toplevel_definitions.jl on lines 219 and 224.

That is all occurrences (by a search over the entire codebase in VSCode) where a println statement generates an include statement.

[johroj]

A related issue is that a/b/c and a\\b\\c are considered as different when comparing explicitly requested files vs. dependencies, which can lead to duplicated includes in the module files. I'm currently working on a proposal for this and will have a look at this issue as well.

I think the proper solution is to make sure / is used everywhere, both internally and when writing to files. This should already be the case on unix and windows understands / too.

[asprionj-NO]

Are we sure that / is "generic enough"? Why not go for a "really generic" solution and use a Vector{String} to convey path information? Get there by splitpath(path::AbstractString) -> Vector{String}, and generate code that uses joinpath(parts::AbstractString...) statements, into which the parts of the path are interpolated (as string literals). Then julia base takes care of the OS abstraction.

Equality of paths can be checked by == on the two parts::Vector{String}, or even using === on tuple(parts...).

[johroj]

We could try to define a type LogicalPath that wraps a vector of strings as you say. Then joinpath, == etc. could use overloads so thay we have no risk for platform-dependent differences. When printed to an include statement, simply concatenate the parts and add / in between.

I do not agree that we should let julia handle os abstractions - that is the root cause of the issue i referred to. Path separation should be about a logical hierarchy here, so any system dependence is just a room for errors.

I can have a look at this and see if changing the path representation would be possible without the risk of introducing too many new bugs. Otherwise, I think my current proposal would achieve the same thing in practice and should be very safe to implement as it only affects windows behavior.

[asprionj-NO]

Ok, julia does IMO fall flat by not providng the LogicalPath type itself. However, functions such as splitpath or joinpath do (correctly) abstract OS-specific aspects. So, in generated code, I would actually place a joinpath statement, not hard-code / as path separator.

[johroj]

I am sorry, but i cannot understand. If you use joinpath to generate code, you would get windows separators in the include statments. Wasn't that the very thing you wanted to get rid of?

Fully agree that this should be provided in Base though.

[asprionj-NO]

I am sorry, but i cannot understand. If you use joinpath to generate code, you would get windows separators in the include statments. Wasn't that the very thing you wanted to get rid of?

Fully agree that this should be provided in Base though.

No, don't use joinpath to generate the code, but have a joinpath statement in the generated code. Is I indicated in the example in my second post; something like:

println(io, reduce((str, spp) -> str*"\"$spp\",", splitpath(script_name), init="joinpath(")[1:end-1]*")")

For relative paths that should all work fine. I'm not sure about absolute paths that include, in Windows, the drive letter. We would have to carefully analyse and test that, if absolute file-system paths ever end up in the generated code.

[johroj]

Thanks, now I get it.

It looks like / is good enough for Base.jl (https://github.com/JuliaLang/julia/blob/8fd607a7df9e65ed060c88f32d801c95aa8a573f/base/Base.jl#L53) so I do not think there is any need to worry about this.

I tested to run protojl with @show proto_script_name(file) in line 172. It just shows "foo_pb.jl", i.e. it is not an absolute path as per your assumption. Have you seen a case where this would be an absolute path?

[johroj]

I realized I could remove joinpath_unix and just hard-code the include paths in the unit tests of my pull request #286. If these tests are merged, we should have a guarantee that the paths are identical on all platforms, thus portable. Does these tests look like a good description of what the output should be to you? (How we achieve this internally is a separate topic.)

If there is any case where an absolute path may end up in the output, we should probably do it system-dependent as you say. Absolute paths are not portable anyway.

Edit: After a quick search in the tests, I could not find anything that seems to be related to absolute paths.

[asprionj-NO]

I think absolute paths could only result from external packages and non-package dependencies. But I have not looked into these at all so far.

The changed (and added) tests in your PR seem to make sense, but don't have time for a deep dive right now. Just having module-local functions named after their Base.* equivalent seems a bit awkward... but why not ;). Still, someone could still write Base.joinpath(...) and get away with it.

[johroj]

Yeah, its not the most beautiful solution, but it helps ensure that there is a unique representation of each path internally, which solves my other issue.

Worst case, we may have to add some keyword argument to control the exported path style.

[johroj]

@Drvi would you mind sharing your opinion on these topics? Is anything related to absolute paths currently supported or planned? Is the solution proposed in #286 satisfactory? Should it be rewritten to use some private helper type to represent paths, despite it probably requiring some larger refactoring? We could either make a small helper type, or use an external library like https://github.com/rofinn/FilePathsBase.jl (I think we should avoid this if a few helper functions can do the same job). I'd be happy to assist, but I think some guidance is necessary.