Skip to content

Add managed build client - #884

Merged
agocke merged 1 commit into
dotnet:masterfrom
agocke:AddManagedBuildClient
Mar 4, 2015
Merged

Add managed build client#884
agocke merged 1 commit into
dotnet:masterfrom
agocke:AddManagedBuildClient

Conversation

@agocke

@agocke agocke commented Feb 26, 2015

Copy link
Copy Markdown
Member

Re-adds the managed client to the build task. Previously the managed client was removed since we wanted to keep the native client around for startup time. However, not spinning up extra processes was a significant perf win and we're now convinced that the native client is unnecessary, so this is the first step in bringing the managed client back.

After reinserting the managed client into the build task, I will delete the native clients and build new csc2/vbc2 executables that perform their same function using the managed client code. When we're confident that works, I will replace the csc2/vbc2 exes with a '/shared' option on the csc/vbc exes and delete csc2/vbc2 entirely (fixing up the tests as appropriate).

Right now the build task references the CodeAnalysis DLL because there are significant helper functions in the assembly which are useful and not easy to extract. If having an independent task is important we should refactor those useful pieces (specifically, some command line argument parsing and C# identifier matching) into linkable pieces.

@jaredpar @AlekseyTs @gafter @VSadov @VladimirReshetnikov @pharring

@agocke

agocke commented Feb 26, 2015

Copy link
Copy Markdown
Member Author

Also, this pull request includes the commit from #866, so that PR should be merged first.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are you missing an argument to match the {0} ?

@agocke

agocke commented Feb 26, 2015

Copy link
Copy Markdown
Member Author

Note, you should ignore the first commit for review purposes, as well, since it's in #866. This should significantly reduce the review size.

@agocke
agocke force-pushed the AddManagedBuildClient branch from 1ff6149 to 11daace Compare February 26, 2015 19:25
@gafter

gafter commented Feb 27, 2015

Copy link
Copy Markdown
Member

👍

@agocke
agocke force-pushed the AddManagedBuildClient branch from 11daace to 00acdb9 Compare March 2, 2015 22:08
@agocke

agocke commented Mar 2, 2015

Copy link
Copy Markdown
Member Author

Could I get one more sign off here?

@agocke

agocke commented Mar 3, 2015

Copy link
Copy Markdown
Member Author

@VladimirReshetnikov I think I've addressed your feedback -- unfortunately it seems Github doesn't want to update the comments for the new diffs.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

AFAIK, Dispose() does not call ReleaseMutex(). Should ReleaseMutex() be within a finally block?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I mistakenly thought dispose releases the mutex. This should be in a finally block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants