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

Question mode usage feedback updates. #9155

Merged
merged 13 commits into from
Nov 18, 2023

Conversation

yuehuang010
Copy link
Contributor

@yuehuang010 yuehuang010 commented Aug 22, 2023

Updating a task behavior to avoid False positives.

After using /question switch on several repro, I am changing a few tasks to reduce False Positive. These task will continue perform there operations as in a normal build. If they impact the incremental build, then it would appear in downstream error. These task should log there file activity so that it could be traced.

Touch and Delete tasks - demote error to warning.

  • Touch is being used to signal processes outside of the build. IE, After build completion, a file is touched to let the other services know work is completed.
  • In C++, touch/delete is used to detect if the build has reached the end or has errored half way through. Using timestamp of the primary output is not a accurate because the build can fail in the post build steps.

WriteLinesToFile Task - remove questioning when WriteOnlyWhenDifferent is false.

  • A common operation is to use this task to write to status log file which are always appending.
  • The Unit Tested already assumed WriteOnlyWhenDifferent is true, so no tests change are needed.

GenerateResource Task - Fix missing message.

  • Note: the detail messages explaining the inputs and outputs are already printed in low priority. This change converts the Log.Message of the task being executed to a error.

@yuehuang010 yuehuang010 marked this pull request as ready for review August 22, 2023 21:52
@YuliiaKovalova
Copy link
Member

@yuehuang010 , could I ask you to fix the build?
Thank you!

src/Tasks/Delete.cs Outdated Show resolved Hide resolved
src/Tasks/Touch.cs Outdated Show resolved Hide resolved
src/Tasks/FileIO/WriteLinesToFile.cs Show resolved Hide resolved
@JanKrivanek JanKrivanek self-requested a review October 26, 2023 14:15
@AR-May AR-May removed the request for review from MichalPavlik October 26, 2023 14:16
@JanKrivanek
Copy link
Member

Spinned of from the #8881 changes

@YuliiaKovalova
Copy link
Member

Hi @yuehuang010,

The PR looks good to me, but could you reply to @Forgind questions above?

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I'm still opinionated on having a dedicated error resource string for failed incrementality check. If that's added - I'll sign off. I'm fine to be voted-off (hence I'm not requesting the change). Or if there is a good reason not to have a separate string for that - I'm ready to listen for reasons.

src/Tasks/GenerateResource.cs Show resolved Hide resolved
src/Tasks/FileIO/WriteLinesToFile.cs Show resolved Hide resolved
@yuehuang010 yuehuang010 changed the title Question feedback changes. Question mode usage feedback updates. Oct 26, 2023
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

I have couple comments - but those are just minor suggestions for consideration. Feel free to resolve those.
Other than that - I'm happy with the current state.

Thank you!

src/Build/BackEnd/Components/Logging/LoggingService.cs Outdated Show resolved Hide resolved
src/Build/BackEnd/Components/Logging/LoggingService.cs Outdated Show resolved Hide resolved
src/Build/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Build/Resources/Strings.resx Outdated Show resolved Hide resolved
@JanKrivanek JanKrivanek merged commit 7595b3a into dotnet:main Nov 18, 2023
8 checks passed
@yuehuang010 yuehuang010 deleted the main_question3 branch February 22, 2024 22:28
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.

4 participants