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

meson: add color to clang-cl #11648

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

meson: add color to clang-cl #11648

wants to merge 2 commits into from

Conversation

neheb
Copy link
Contributor

@neheb neheb commented Apr 4, 2023

Both fcolor-diagnostics and fansi-escape-codes are needed

Both fcolor-diagnostics and fansi-escape-codes are needed

Signed-off-by: Rosen Penev <rosenp@gmail.com>
@neheb neheb requested a review from dcbaker as a code owner April 4, 2023 04:59
@neheb
Copy link
Contributor Author

neheb commented Apr 4, 2023

CI failures seem unrelated.

-openmp:llvm is needed to link.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
@@ -62,6 +62,12 @@
'neon': None,
} # T.Dicst[str, T.Optional[T.List[str]]]

clang_color_args = {
'auto': ['-fcolor-diagnostics', '-fansi-escape-codes'],
'always': ['-fcolor-diagnostics', '-fansi-escape-codes'],
Copy link
Member

Choose a reason for hiding this comment

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

This does not seem right. auto and always do not mean the same thing, thus it is hard to believe they can be implemented in the same way. auto means use colors unless stdout is not a terminal, always means always use colors. Using the same compiler options for both means that either you get escape sequences emitted when stdout is not a tty when auto is used, or you don't get them in the same situation when always is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is copied from what is present in clang

Copy link
Member

Choose a reason for hiding this comment

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

If I read correctly the documentation https://clang.llvm.org/docs/UsersManual.html#cmdoption-f-no-color-diagnostics this patch is not necessary. In particular, I think that enabling ansi escape codes on Windows is a bad idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

Because the Windows terminal does not understand ansi escape codes.

This comment was marked as off-topic.

Copy link
Member

Choose a reason for hiding this comment

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

Not listening to anything ChatGPT says, it's not a trustworthy source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's answer was correct here. cmd.exe does support ANSI escape codes.

Copy link
Member

Choose a reason for hiding this comment

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

It supports it if you explicitly enable it. It is not the default, thus it cannot be Meson default to emit ansi escape codes that will be shown as unreadable output for most users. Also, I don't think that enabling the ansi escapes codes in the Windows 10 command prompt disables the Windows Console API, thus I don't see why it is desirable to enable this option by default. The commit message says that the -fansi-escape-codes is required, but it does not look like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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