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

Enable compiler_param_file in Windows MSVC toolchain #17135

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Jan 4, 2023

Fixes #5163

@fmeum
Copy link
Collaborator Author

fmeum commented Jan 4, 2023

@oquenchil The tests fail as they inspect the logs and search for compiler flags. Should I make them look into the param file or, probably better, inspect aquery output instead?

@oquenchil
Copy link
Contributor

@oquenchil The tests fail as they inspect the logs and search for compiler flags. Should I make them look into the param file or, probably better, inspect aquery output instead?

I was going to suggest that usually the way we do this is to get to the parameter file writing action and look at the flags there instead. Such a test would be easily converted to a Starlark test. But specifically for compiler_param_file there isn't a separate action, so as you are suggesting, we need a strategy that is part of an integration test.

Do these compiler flags show up in aquery? If they do, that's probably better than reading the parameter file since it avoids execution.

@oquenchil
Copy link
Contributor

Thank you for the very quick PR Fabian.

@fmeum fmeum force-pushed the patch-7 branch 11 times, most recently from 509458f to 199e1b7 Compare January 5, 2023 20:39
@fmeum
Copy link
Collaborator Author

fmeum commented Jan 5, 2023

@oquenchil Tests are passing now. I also added a test for long command lines based on your gist.

@fmeum fmeum marked this pull request as ready for review January 5, 2023 21:05
@ShreeM01 ShreeM01 added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Jan 6, 2023
Copy link
Contributor

@oquenchil oquenchil left a comment

Choose a reason for hiding this comment

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

Thank you Fabian!

@oquenchil oquenchil added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 9, 2023
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 11, 2023
@fmeum fmeum deleted the patch-7 branch January 11, 2023 10:27
hvadehra pushed a commit that referenced this pull request Feb 14, 2023
Fixes #5163

Closes #17135.

PiperOrigin-RevId: 501212729
Change-Id: I6b4371a5521230c4c89f92321933b578bf5c3051
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compile Line Length Limit hit under Windows
5 participants