Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork114
Cache gcc warnings#302
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
matthijskooijman left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Overall, this looks good to me. I did leave quite some smaller remarks inline.
I don't understand the concurrency issue exactly: If this interleaves output from multiple concurrent processes, then why doesn't that happen in the original compilation run? Is that perhaps just a coincidence, that the chances of interleaving are small when each compilation command actually takes some time? Or is there some mechanism in place to actually properly interleave output from multiple processes? I can actually imagine that, for multi-process compilation, it would make sense to (only) capture all output from a single compilation command and then write that output atomically (rather than letting the compiler write to os.stdout/stderr directly, with the risk of interleaving)?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
builder_utils/utils.go Outdated
| if!objIsUpToDate { | ||
| _,_,err=ExecRecipe(ctx,properties,recipe,false/* stdout */,utils.ShowIfVerbose/* stderr */,utils.Show) | ||
| _,stderrCache,err:=ExecRecipe(ctx,properties,recipe,false/* stdout */,utils.ShowIfVerbose/* stderr */,utils.Show) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
stderrCache implies (to me) that this is a previously cached value, whereas I believe this is the valueto be cached. Perhapsstderr orerrOutput is a better name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
builder_utils/utils.go Outdated
| } | ||
| }elseifctx.Verbose { | ||
| logger.Println(constants.LOG_LEVEL_INFO,constants.MSG_USING_PREVIOUS_COMPILED_FILE,properties[constants.BUILD_PROPERTIES_OBJECT_FILE]) | ||
| ctx.WarningsCache[source]=string(stderrCache) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we convert this to string? Can we not marshal a byte slice in JSON? Converting to string here, requires the use of UTF-8 (I think, at least). Probably not a problem in practice, but just keeping this a byte slice could be more transparent?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
facchinm commentedOct 15, 2018
@matthijskooijman there's a mutex that prevents interleaving when printing via |
ArduinoBot commentedOct 16, 2018
✅ Build completed. ⬇️ Build URL: ℹ️ To test this build:
|
facchinm commentedOct 16, 2018
@matthijskooijman I think I addressed all you concerns. At the end the interleaving only happens when the streams are crunched by the Java IDE (I'll take a closer look later), but they're fine when launched on a terminal. |
matthijskooijman left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good. Two more minor remarks added.
Are you planning to squash some of thes commits together before merging?
utils/utils.go Outdated
| Show=1// Show on stdout/stderr as normal | ||
| ShowIfVerbose=2// Show if verbose is set, Ignore otherwise | ||
| Capture=3// Capture into buffer | ||
| Show=1// Print the stream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Perhaps clarify that Show and ShowIfVerbose captures and then prints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, I'm planning to cleanup the patchset as soon as all the issues are fixed 😉 Good catch!
types/context.go Outdated
| CodeCompletionsstring | ||
| WarningsLevelstring | ||
| WarningsCachemap[string]string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess this should also be renamed to OutputCache or CompilerOutputCache or something?
This commits doesn't change the functionality of ExecCommand EXCEPT for programs that show progress bars or similar.For this kind of utilities, please use Stream "type"
This file should cache warnings and other compiler output between compilations.Directly maps to ctx.OutputCache map
The mutex is needed because the map could be concurrently accessed (when using parallel compilation)
ArduinoBot commentedOct 16, 2018
✅ Build completed. ⬇️ Build URL: ℹ️ To test this build:
|
... and display them even if the file is not being recompiled.
Should allow removing48aa042 from#301
There's a concurrency issue on cached prints that should be solved before merging.