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

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 August 4, 2020 15:02
Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 August 13, 2020 05:49
Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems OK

@LordAro LordAro merged commit 6358ae4 into OpenTTD:master Sep 1, 2020
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.

None yet

3 participants