n2 icon indicating copy to clipboard operation
n2 copied to clipboard

Support Windows (backslashed) paths

Open evmar opened this issue 3 years ago • 6 comments

On Windows paths can contain either / or \. It's important for n2 to recognize all references to a file are the same, even if they're written in different ways. I think the failure modes that can come up are like

  1. in the build file, it's written foo\bar.h
  2. in the dependency output (e.g. depfile or /showIncludes) it's written foo/bar.h

It doesn't work to just always canonicalize paths to unix-style because a foo\bar.h in a build line might forward in to a command line to a tool that doesn't understand foo/bar.h. I believe because of this Ninja goes to effort to keep track of where the slashes were in a path.

Here's an idea I had: what if we preserved the slashes found in the input, but then when looking a path string up we allowed back and forward slashes to both match the same File? Basically tweak this around here, at the hash lookup time? @sgraham do you remember this stuff?

evmar avatar Apr 06 '22 14:04 evmar

To me that sounds as if the build file is the ground truth and the dependency output could be noisy?!? Are there different spellings of the same file in the build file?

tschuett avatar Apr 06 '22 18:04 tschuett

I think there can be different spellings in the build file, depending on the file. In other words, I think because Ninja canonicalizes, there might be build files out there that spell the same file differently in different places. We could consider rejecting those files though, given that in principle you could always fix your generator to generate your build files "right". (Though how to actually identify that a file is in that state to reject it is itself an interesting question...)

@clemenswasser can you share a Windows format CMake-generated build.ninja/rules.ninja somewhere so we could look at it?

emartinfigma avatar Apr 06 '22 20:04 emartinfigma

Well, build tools could ask for different spellings including a full path.

tschuett avatar Apr 06 '22 22:04 tschuett

(Sorry, not at a computer during the day these days.)

I'm not too sure any more why we funneled it through CanonicalizePath() rather than pointing at a File (Node) as a interning step. I was wondering about maybe trying to maintain the right specific messy slashes for each individual instance of a file name so that a different command invocations would get the slashes they expected. But I don't think ninja does that anyway, because slash_bits are stored in the Node (based on the first-found slash_bits), so collapsing them means it'd only know about a single slash style in any case.

So my guess is a less grand explanation, along the lines of "it seemed like the way to make the change as small as possible at the time".

(btw, cool! ninja was due for a ninja-ing. :)

sgraham avatar Apr 07 '22 03:04 sgraham

Thinking about this more, I think my idea would be much easier to implement then needing to tweak a hash function. If you look at the point I linked above, we clone the path twice, and the second one is for the hash table. On Windows right there we could fix up the second one’s slashes.

evmar avatar Apr 07 '22 14:04 evmar

https://github.com/ninja-build/ninja/issues/843 has some background on why ninja does what it does. We tried canonicalizing everything to / first, but cmake's cmcldeps tries to find the /fo switch and just basically does a strstr() for it instead of commandline parsing, and then e.g. compiling a file in the folder media/formats/.. would suddenly be interpreted as /fo flag, while it happens to "work" with backslashes.

If ninja keeps the input slashiness, it kind of punts responsibility to the user, and then it's at least possible to have a build that works.

cmcldeps still does this! https://cs.github.com/Kitware/CMake/blob/6e1be5dbefab3e7317502e3d0fe4b132d0162ae5/Source/cmcldeps.cxx?q=%22%2FFo#L275

(I'm still salty about this.)

nico avatar Apr 07 '22 20:04 nico