search

dawnbeen / c_formatter_42

C language formatter for 42 norminette
GNU General Public License v3.0
176 stars 17 forks source link

✨ Add support for Windows (#30) #32

Closed younesaassila closed 2 years ago

younesaassila commented 2 years ago
younesaassila commented 2 years ago

Rebased the branch to include the linter error fix. I think because my 1st PR is from the master branch this one also includes its changes, so I think the 1st one needs to be merged first before this PR 🤔

cacharle commented 2 years ago

I'll verify tomorrow that the clang executable you provided is safe.

Thanks for fixing that issue aswell btw

cacharle commented 2 years ago

I also wonder if we have to provide an executable for windows 64bit next to the 32bit version you provided.

I don't think that was an issue for linux or mac tho..

younesaassila commented 2 years ago

I also wonder if we have to provide an executable for windows 64bit next to the 32bit version you provided.

I don't think that was an issue for linux or mac tho..

I tested the 32-bit executable on a 64-bit machine without issues. The "win32" name in the Python code doesn't refer to the architecture but to Windows itself (just like "darwin" refers to macOS).

cacharle commented 2 years ago

Can you provide an official download link to the exact executable you added?

I don't know which clang-format version you added (tried against 12.0 and the latest but no match).

younesaassila commented 2 years ago

Can you provide an official download link to the exact executable you added?

I don't know which clang-format version you added (tried against 12.0 and the latest but no match).

@cacharle I don't believe there's a direct link to a clang-format binary for Windows. The best way to download it (c.f. the many StackOverflow posts about it) is to install the LLVM compiler (safe) at https://llvm.org/builds/ and get the bundled clang-format binary from C:\Program Files (x86)\LLVM\bin\clang-format.exe. (You may use Windows Sandbox to create a virtual machine to install LLVM).

The version of the clang-format binary included in my commit is v12.0.0. You may check this out with the clang-format-win32.exe --version command.

cacharle commented 2 years ago

Are you sure we can't just download the binaries from the github release page https://github.com/llvm/llvm-project/releases/download/llvmorg-12.0.0/LLVM-12.0.0-win32.exe?

Is it not standalone?

I just tried to download it from there for Mac and it seems to work on his own.

younesaassila commented 2 years ago

@cacharle You're right, we can download LLVM directly from their GitHub repo. In fact, the version on their website is from two years ago 😂 However there doesn't seem to be a clang-format standalone, so we still need to install LLVM on a virtual machine and copy the clang-format.exe from the bin directory of the LLVM install.

I downloaded LLVM from their GitHub repo and the clang-format executable is version 14.0.6. Would you like me to update the executable I included to this version or should we keep version 12? (I don't know what version the clang-format for Mac and Linux are, sorry)

cacharle commented 2 years ago

The mac and linux executables are version 12 so you can keep it that way.

Can you provide a direct link to the thing you downloaded then (in which I can see the executable) ?

younesaassila commented 2 years ago

The mac and linux executables are version 12 so you can keep it that way.

Can you provide a direct link to the thing you downloaded then (in which I can see the executable) ?

Sure: https://prereleases.llvm.org/win-snapshots/LLVM-12.0.0-6923b0a7-win32.exe

younesaassila commented 2 years ago

Hey,

I checked the version of clang-format-darwin and clang-format-linux and both are 11.0.0. For consistency, should we downgrade the version of clang-format-win32.exe to 11.0.0 or upgrade the version of clang-format-darwin and clang-format-linux to 12.0.0? (From what I can tell from the commits in llvm-project/clang/tools/clang-format/ there doesn't seem to be breaking changes from 11 to 12 and c_formatter_42's tests ran successfully on Windows when using version 12)

Also, like you pointed out, it would be better to retrieve the clang-format-win32.exe executable from LLVM's GitHub page. The URL I shared in my previous message contains "prereleases" and I'm worried the version of LLVM I extracted the binary from might be pre-release software.

Edit: clang-format-win32.exe is currently version 14.0.6?! I'll retrieve version 11 from LLVM's GitHub page and commit it. Link: https://github.com/llvm/llvm-project/releases/download/llvmorg-11.0.0/LLVM-11.0.0-win32.exe

younesaassila commented 2 years ago

Also, if Windows support is added, CI tests should run on Windows as well