-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Use *W versions of win32 file functions #2289
base: master
Are you sure you want to change the base?
Conversation
So this PR works together with the long path enablement in Windows and the manifest, correct? |
So that the long paths work on windows with registry LongPathsEnabled=1
5e85f4d
to
c1fd2bc
Compare
@emmenlau yes, at least for my case. We use response files, and ninja was unable to create the files without this patch. Now my next problem is that |
We happily switched to |
It wasn't empty |
@cristianadam It seems reasonable that Looks legit to me. |
The docs explicitly state the functions that the manifest allows long paths for in conjunction with |
I agree. However @cristianadam tested his change in #2225 (comment) and found that it works for him. It did work for me on our desktop computer, too. It only failed on our CI machine for me. So there seems to be at least a small step forward, but it is unclear why it is not a full solution. Maybe this is the missing link, I'm keeping fingers crossed! |
I'm running into this issue with But ninja itself seems to work fine with this patch. |
any news about merge ? |
It this supposed to fix long paths issues in Windows? It tried it on my project, and I still get the following error:
|
@Osyotr are you still working on this? I.e., comfortable reviewing this PR if I take it over? |
I am not a maintainer of this project. I removed my review comment because I found the answer myself: ninja uses UTF-8 on windows: Line 5 in dcefb83
|
@jhasse would you accept this PR if I got it in shape? |
No, sorry. |
This is a forever nightmare. I guess the only real "solution" is to maintain a fork with this PR, and make binary releases for people to use. We currently maintain this inside our company, but it's obvious that more people need this. Volunteers? I can also chime in after my holidays in two weeks... |
I don't get it - you think the current state (where everyone has to move their build dir to |
you should follow up the "no, sorry" with an alternative that would be considered acceptable... by the way what's the reason? is it compatibility? here's a random idea:
|
@makslevental and @mpratt14 , I share your sentiment! But all of this has been discussed before, and already a few times. Just read the (multiple) corresponding issues and pull requests. Short summary: The core maintainer wants this to be solved in Microsoft layer, not in this project. There is no point in discussing this over and over, unless there is new insight. Either provide a PR that solves it in the Microsoft layer (like the already merged PR tries to do) or fork the project and maintain a patch with some other kind of workaround. Just my two cents. |
Please do try the latest ninja release (1.21.1) and make sure your Windows has Ninja has now the manifest entry mentioned at #2225 and it should be able to read and write files longer than 260 characters. So there is no need for a There is a BIG issue though, Windows cannot execute processes in paths longer than 260 characters (due to Windows has historical limitations and you need to use |
Nice explanation I see that I was not suggesting that this PR is good, but like I said, there must be something we can do. |
So that the long paths work on windows with registry LongPathsEnabled=1