Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize the dotnet wrapper a bit #131

Merged
merged 1 commit into from
Mar 7, 2020

Conversation

j3parker
Copy link
Member

@j3parker j3parker commented Mar 7, 2020

csharp_binary needs to become a rule which emits a cc_binary that
invokes dotnet <foo>, where <foo> is the actual dll that we
compile, to solve #71. Eventually <foo> will become a wrapper C# exe
that tweaks assembly loading to solve #9. So all of this requires the
wrapper to provide a default argv[1].

For stuff like dotnet vstest (#51) we will have argv[1] be vstest, so we
also need support for baking in argv[2].

@j3parker j3parker requested a review from omsmith March 7, 2020 21:28
@j3parker j3parker force-pushed the dotnet-wrapper-generalize branch from 5579d3f to 3ad623a Compare March 7, 2020 21:32
@j3parker
Copy link
Member Author

j3parker commented Mar 7, 2020

Er, had this sitting around on a branch but I forgot to tidy some stuff up.

@j3parker j3parker force-pushed the dotnet-wrapper-generalize branch 4 times, most recently from 1200f1c to 8840f29 Compare March 7, 2020 21:57
@j3parker
Copy link
Member Author

j3parker commented Mar 7, 2020

Ok.

@j3parker
Copy link
Member Author

j3parker commented Mar 7, 2020

Improvements for future PRs:

  • Move template params out to a header to make it obvious where they are
  • Remove the ifdefs in favour of Starlark select() to chose the write helper method impl
  • Switch to CreateProcess (the correct low-level thing) from spawnv on Windows.

omsmith
omsmith previously approved these changes Mar 7, 2020
csharp/private/wrappers/dotnet.cc Show resolved Hide resolved
csharp/private/wrappers/dotnet.cc Outdated Show resolved Hide resolved
csharp_binary needs to become a rule which emits a cc_binary that
invokes "dotnet <foo>", where "<foo>" is the actual *.dll that we
compile, to solve Brightspace#71. Eventually <foo> will become a wrapper C# exe
that tweaks assembly loading to solve Brightspace#9. So all of this requires the
wrapper to provide a default argv[1].

For stuff like dotnet vstest (Brightspace#51) we will have argv[1] = vstest, so we
also need support for baking in argv[2].
@j3parker j3parker force-pushed the dotnet-wrapper-generalize branch from 8840f29 to a3564d6 Compare March 7, 2020 22:18
@j3parker j3parker merged commit 5531b47 into Brightspace:master Mar 7, 2020
@j3parker j3parker deleted the dotnet-wrapper-generalize branch March 7, 2020 22:19
dotnet_argv[0] = const_cast<char*>("dotnet");

// Make sure to hold a reference to these std::string.
auto argv1 = runfiles->Rlocation(kArgv1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants