- Notifications
You must be signed in to change notification settings - Fork548
[mtouch] Detect when we run into the 32-bit arm size limitation, and report a better error. Fixes #6526.#6855
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…report a better error.Fixesdotnet#6526.Also limit the output from the native compiler, so that we don't overload theIDEs with output if the native compiler produces tens of thousands of errors.Fixesdotnet#6526.
spouliot 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.
minor
tools/common/Driver.cs Outdated
| varmsg=$"Process exited with code{p.ExitCode}, command:\n{path}"; | ||
| if(output!=null&&output.Length>0) | ||
| msg+=$"\n{output}"; | ||
| Console.Error.WriteLine(msg); |
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.
why not just do aConsole.Error.WriteLine (output); to avoid creating a new (possibly very large) string ? e.g.
Console.Error.WriteLine($"Process exited with code{p.ExitCode}, command:\n{path}");if(output!=null&&output.Length>0){Console.Error.WriteLine();Console.Error.WriteLine(output);}
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.
There are other comments that explain this: we do things in parallel, and having multiple CWLs means it will be much more likely that other threads will write to stdout at the same time, interspersing their output with this output, and making a big incomprehensible mess out of it all.
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.
yep, seen it (later) but maybe we could re-use theStringBuilder instance here ? e.g. callInsert to pre-pendmsg and a new line and then a singleToString withWriteLine ?
tools/mtouch/BuildTasks.mtouch.cs Outdated
| // There can be thousands of these, but we only need one. | ||
| reported_5107=true; | ||
| exceptions.Add(ErrorHelper.CreateError(5107,"The assembly '{0}' can't be AOT-compiled for 32-bit architectures because the native code is too big for the 32-bit ARM architecture.",Path.GetFileNameWithoutExtension(OutputFile))); | ||
| } |
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.
can't we share that same logic instead of having it in two places ?
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'll look into that
monojenkins commentedAug 27, 2019
Build success |
spouliot 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.
❤️
monojenkins commentedAug 27, 2019
Build failure Test results56 tests failed, 37 tests passed.Failed tests
|
| mtouch.Profile=Profile.iOS; | ||
| mtouch.Abi="armv7"; | ||
| mtouch.Linker=MTouchLinker.DontLink; | ||
| /* Once the xcode11 branch has been merged into master, we should be able to do the following instead, which will make the test faster |
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.
Issue to remember doing that?
monojenkins commentedAug 28, 2019
Build success |
Also limit the output from the native compiler, so that we don't overload the
IDEs with output if the native compiler produces tens of thousands of errors.
Fixes#6526.