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

Fix clang-cl build on Windows #8289

Merged
merged 1 commit into from Sep 1, 2020
Merged

Fix clang-cl build on Windows #8289

merged 1 commit into from Sep 1, 2020

Conversation

@Adriankhl
Copy link
Contributor

@Adriankhl Adriankhl commented Aug 4, 2020

llvm on Windows does not come with Editbin (In general, I avoid using MSVC tools for clang build on Windows, except a few libraries and headers.), and clang-cl does not take the /MP flag.

Would it cause any problem without the Editbin? It seems like it is only used in cmake/scripts/Regression.cmake

@LordAro LordAro requested a review from glx22 Aug 4, 2020
Copy link
Contributor

@glx22 glx22 left a comment

The /MP part is ok (removes a lot of useless warnings), but editbin is required for regression tests to pass. We use editbin to temporary convert openttd into a console app, so we can capture and redirect output to a file.

Oh and the commit message doesn't respect coding style

@Adriankhl
Copy link
Contributor Author

@Adriankhl Adriankhl commented Aug 5, 2020

Now, I modified FindEditbin.cmake such that it uses $ENV{VCToolsInstallDir} to search for editbin.exe if the compiler is clang-cl. VCToolsInstallDir is automatically defined in developer powershell/cmd, I personally extract some environmental variables from developer cmd for Git Bash and build stuffs via Git Bash shell, so the build should work in both shells. People can also specify EDITBIN_DIRECTORY if it is located in some other directories.

I have also tried ninja test and it looks like it works.

@LordAro LordAro requested a review from glx22 Aug 13, 2020
glx22
glx22 approved these changes Aug 13, 2020
Copy link
Contributor

@glx22 glx22 left a comment

Seems OK

@LordAro LordAro merged commit 6358ae4 into OpenTTD:master Sep 1, 2020
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants