Skip to content
This repository was archived by the owner on Dec 18, 2017. It is now read-only.

Suppress ICompilationException coming from EntryPointExecutor.Execute #2962

Merged
merged 1 commit into from
Oct 13, 2015

Conversation

pakrym
Copy link
Contributor

@pakrym pakrym commented Oct 12, 2015

@@ -221,13 +221,30 @@ private bool ParseArgs(string[] args, out RuntimeOptions defaultHostOptions, out
return false;
}

private Task<int> ExecuteMain(DefaultHost host, string applicationName, string[] args)
private async Task<int> ExecuteMain(DefaultHost host, string applicationName, string[] args)
Copy link
Contributor

Choose a reason for hiding this comment

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

don't make it async

@davidfowl
Copy link
Member

I don't quite understand what this is fixing. Can you show all of the cases that cause compilation errors and how this fixes each of them?

@pakrym
Copy link
Contributor Author

pakrym commented Oct 12, 2015

@davidfowl
There are two source of compilation errors:

  1. When we are searching for EntryPoint method and compiling application code on the way (host.GetEntryPoint)
  2. Some code in application EntryPoint is doing compilation and throws compilation exception (AspNet hosting is searching for Startup type which causes related projects to compile). That was the case that didn't work for you here: Change Roslyn complation exception search #2928 (comment)

I moved EntryPointExecutor.Execute into try/catch that handles ICompilationException stack suppressing, so stack traces from entry point execution would be also hidden.

@@ -250,8 +240,6 @@ private Task<int> ExecuteMain(DefaultHost host, string applicationName, string[]
applicationName,
ex.InnerException);
}

throw;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this throw still needed? I think without this we will suppress all exceptions but FileLoad/FileNotFound and compilation exceptions...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@moozzyk
Copy link
Contributor

moozzyk commented Oct 12, 2015

🚢🇮🇹 (after you confirm we are not wrapping application exceptions in AggregateExceptions and Travis passes)

@pakrym pakrym force-pushed the pakrym/2887-ExecuteMain branch from 63edeee to 9cd75c9 Compare October 13, 2015 20:45
@pakrym pakrym force-pushed the pakrym/2887-ExecuteMain branch from 9cd75c9 to 1b08f54 Compare October 13, 2015 23:37
@pakrym pakrym merged commit 1b08f54 into dev Oct 13, 2015
@pakrym pakrym deleted the pakrym/2887-ExecuteMain branch October 13, 2015 23:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants