Cache gcc warnings #302
Cache gcc warnings #302
Conversation
There was a problem hiding this comment.
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)?
builder_utils/utils.go
Outdated
| @@ -230,12 +230,18 @@ func compileFileWithRecipe(ctx *types.Context, sourcePath string, source string, | |||
| } | |||
|
|
|||
| 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.
stderrCache implies (to me) that this is a previously cached value, whereas I believe this is the value to be cached. Perhaps stderr or errOutput is a better name?
builder_utils/utils.go
Outdated
| if err != nil { | ||
| return "", i18n.WrapError(err) | ||
| } | ||
| } else if ctx.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.
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?
|
@matthijskooijman there's a mutex that prevents interleaving when printing via |
|
✅ Build completed. ⬇️ Build URL: ℹ️ To test this build:
|
|
@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. |
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.
Perhaps clarify that Show and ShowIfVerbose captures and then prints?
There was a problem hiding this comment.
Yes, I'm planning to cleanup the patchset as soon as all the issues are fixed 😉 Good catch!
types/context.go
Outdated
| @@ -65,6 +65,7 @@ type Context struct { | |||
| CodeCompletions string | |||
|
|
|||
| WarningsLevel string | |||
| WarningsCache map[string]string | |||
There was a problem hiding this comment.
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)
facchinm
force-pushed
the
warning_cache
branch
from
4006605 to
672b006
Compare
|
✅ Build completed. ⬇️ Build URL: ℹ️ To test this build:
|
... and display them even if the file is not being recompiled.
Should allow removing 48aa042 from #301
There's a concurrency issue on cached prints that should be solved before merging.