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

Use *W versions of win32 file functions #2289

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

parbo
Copy link

@parbo parbo commented Apr 21, 2023

So that the long paths work on windows with registry LongPathsEnabled=1

@emmenlau
Copy link

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
@parbo
Copy link
Author

parbo commented Apr 21, 2023

@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 cl is unable to open the created response file.. 😢

@emmenlau
Copy link

Now my next problem is that cl is unable to open the created response file.. cry

We happily switched to clang-cl.exe, which in my eyes is the much better compiler, and is trivially simple to switch from cl.exe. It works with long paths, too.

@parbo
Copy link
Author

parbo commented Apr 21, 2023

I think the problem seems to be that the response file that gets created is empty, so there might be some issue with my patch.

It wasn't empty

@emmenlau
Copy link

@cristianadam It seems reasonable that *W methods should be used in combination with the manifest. Could this be the missing link why the previous PR alone did not solve the problem?

Looks legit to me.

@parbo
Copy link
Author

parbo commented Apr 21, 2023

The docs explicitly state the functions that the manifest allows long paths for in conjunction with LongPathsEnabled. It's only the *W functions, no *A functions mentioned.

@emmenlau
Copy link

The docs explicitly state the functions that the manifest allows long paths for in conjunction with LongPathsEnabled. It's only the *W functions, no *A functions mentioned.

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!

@parbo
Copy link
Author

parbo commented Apr 21, 2023

I'm running into this issue with cl.exe: https://developercommunity.visualstudio.com/t/clexe-command-line-error-d8022-cannot-open-path-re/1478916

But ninja itself seems to work fine with this patch.

@Koxx3
Copy link

Koxx3 commented Jun 26, 2023

any news about merge ?

@bruchar1
Copy link

It this supposed to fix long paths issues in Windows? It tried it on my project, and I still get the following error:

ninja: error: WriteFile(src/monorepo/lib/LineMasterUI/LineMasterUI-vc143-mt-gd-x64-u.dll.p/MainWindow_AdministratorOptions_AdvancedOptions_AutomaticDispositionChange_AutomaticDispositionChangeWidget.cpp.obj.rsp): Unable to create file. The system cannot find the path specified.
ninja: build stopped: .

@makslevental
Copy link

@Osyotr are you still working on this? I.e., comfortable reviewing this PR if I take it over?

@Osyotr
Copy link

Osyotr commented Aug 21, 2024

@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:

<activeCodePage xmlns="http://schemas.microsoft.com/SMI/2019/WindowsSettings">UTF-8</activeCodePage>

@makslevental
Copy link

@jhasse would you accept this PR if I got it in shape?

@jhasse
Copy link
Collaborator

jhasse commented Aug 23, 2024

No, sorry.

@emmenlau
Copy link

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...

@makslevental
Copy link

No, sorry.

I don't get it - you think the current state (where everyone has to move their build dir to C:\b) is the correct state?

@mcprat
Copy link
Contributor

mcprat commented Aug 23, 2024

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:

  1. catch the error and put the files that error into a completely different directory than the build directory, call it something like C:\ninja-extra-<project>\

  2. catch the error again if it happens again and rename the file

@emmenlau
Copy link

@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.

@cristianadam
Copy link
Contributor

Please do try the latest ninja release (1.21.1) and make sure your Windows has Computer\HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\FileSystem\LongPathsEnabled (Type: REG_DWORD) set to 1.

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 *W patch.

There is a BIG issue though, Windows cannot execute processes in paths longer than 260 characters (due to SetCurrentDirectory). I've mentioned this at https://developercommunity.visualstudio.com/t/compiler-cant-find-source-file-in-path/10221576 and it looks like this:

windows11-longpaths-fail

Windows has historical limitations and you need to use subst or Junctions to get things to work. Or just cross-compile with clang-cl from a different operating system.

@mcprat
Copy link
Contributor

mcprat commented Aug 25, 2024

Nice explanation

I see that longPathAware is also added already, which I was just reading about.

I was not suggesting that this PR is good, but like I said, there must be something we can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants