Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Windows (backslashed) paths #42

Open
evmar opened this issue Apr 6, 2022 · 6 comments
Open

Support Windows (backslashed) paths #42

evmar opened this issue Apr 6, 2022 · 6 comments
Labels
enhancement New feature or request windows Windows-specific issues

Comments

@evmar
Copy link
Owner

evmar commented Apr 6, 2022

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?

@tschuett
Copy link
Contributor

tschuett commented Apr 6, 2022

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?

@emartinfigma
Copy link
Contributor

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?

@tschuett
Copy link
Contributor

tschuett commented Apr 6, 2022

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

@sgraham
Copy link

sgraham commented Apr 7, 2022

(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. :)

@evmar evmar added the enhancement New feature or request label Apr 7, 2022
@evmar
Copy link
Owner Author

evmar commented Apr 7, 2022

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.

@nico
Copy link
Contributor

nico commented Apr 7, 2022

ninja-build/ninja#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.)

@evmar evmar added the windows Windows-specific issues label Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request windows Windows-specific issues
Projects
None yet
Development

No branches or pull requests

5 participants