-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Need to rethink GetRunInformation as a Target #98
Comments
Another piece of information to add here is the debugger type: desktop or Core. |
We've decided to go with a "runInformation.json" file that gets generated during the "Build" of a project. Here is my "design" for this file: {
"command": "C:\\Program Files\\dotnet\\dotnet.exe",
"arguments": "exec F:\\source\\MyApp\\bin\\Debug\\netcoreapp1.0\\MyApp.dll",
"workingDirectory": " F:\\source\\MyApp\\bin\\Debug\\netcoreapp1.0",
"debugEngines": "{2E36F1D4-B23C-435D-AB41-18E608940038}",
"environmentVariables": {
"Name1": "Value1",
"Name2": "Value2"
}
} There will be a new MSBuild property that will hold the path to this file named Questions
|
Why was a external file decided? I'll prefer MSBuild properties representing this information. |
@BillHiebert Thoughts on this? How does this fit in with the unification with VS code/Xamarin? |
The thinking behind an external file was that it had the following advantages:
|
BTW - @davkean, if you have better advantages to using all MSBuild properties to represent this information, I can easily be swayed. I just couldn't think of any myself. |
VS has support for a launchSettings.json file which contains similar information: like environment variables and working directory. I guess they would replace the ones specified n the generated file. |
/cc @DustinCampbell - to help with the questions on VS Code. |
@eerhardt where would this external file live? |
It defaults to be
Like everything else in the |
Actually, VS Code already has a separate file. The user can set up tasks.json and launch.json files in a .vscode folder to handle build and debug configuration. The C# extension for VS Code does a lot of work to generate these for the user. These files look very similar to the design above. So, users will need to keep this file in sync with VS Code's assets. In the future, I'm hoping VS Code can allow us to dynamically add launch configurations. That way, we can just read configuration from wherever it is (project file, external file, whatever) and just feed it into VS Code. #10861 is tracking that. FWIW, I'm concerned about this additional file from a concept count. We're trying to reduce concepts, not add more. Why are we inventing new things? Note: @BillHiebert, VS Code doesn't host MSBuild -- OmniSharp does. That's already taken care of. |
@DustinCampbell if I'm reading what you wrote correctly, that would mean for VSCode users the
|
Correct. Essentially, we'd have to have this file to keep other IDEs happy and VS Code's tasks.json and launch.json to keep VS Code happy. |
Users will never read or modify the file that is being proposed - it is generated by the "Build" MSBuild Target, and it is generated into the "obj$(Configuration)$(TFM)" folder.
This is not a user concept at all. This is the "hand-shake" between the MSBuild project and something that wants to "run" the project's output. As it currently sits, there is no abstract way of providing this information, and all callers need to implement the same rules:
The idea here is to put all those rules into the MSBuild project, and consumers who want to run the app (like You can image that VS Code's tasks.json and launch.json could be generated from this file. As it looks today, the current generation won't work for MSBuild projects, since you are hard-coding |
Got it. So, MSBuild will have to be invoked to produce this file? Allow me to walk through the core VS Code scenario:
At this point, we offer to generate a tasks.json and launch.json so that users can F5 successfully. If I understand you correctly, it sounds like there's a bit of a "chicken and egg" problem because the run configuration wouldn't be available until a real Build (i.e. not a DesignTimeBuild) occurs. Am I right about that? |
MSBuild is the only place this information can be retrieved from, so yes, it must be invoked. The user's .csproj could have something like
My current thinking is that the Target that produces this file is hooked into the "Build" target, so the file is always produced during the build. But there is no reason why that Target couldn't be invoked directly by a DesignTimeBuild to produce the file. |
From my perspective, as someone who cares deeply about our overall UX, I would say that I have no problem if we produce this file as long as we can guarantee that the user will never see it and will never have to worry about it in any way or form. @eerhardt invoking that target manually as you suggest, however, would trigger a full build or no? |
@eerhardt There's a difference to evaluation time and build time. Project systems try very hard to avoid building a project (via design-time build) to get data that can be gathered during evaluation (statically). Design-time builds are a magnitude more expensive than evaluations, and in the legacy project system, the cause of 8 of the top 10 UI delays in csproj. I would prefer a static way of retrieving this data - if you want to produce the file based on that, I'm okay, but from project system perspective I don't want a build to retrieve data that be retrieved statically. |
Are we guaranteed this information will always be able to be retrieved statically for all project types that we want to run? It would be very counter-productive to make this static data for C# .NET Core apps, and then the next project type we tried enabling was not able to produce this information statically. (Honestly, even for .NET Core apps, I can imagine someone needing the "ResolveNetCoreHost" Task in order to be able to figure out which "dotnet.exe" host to invoke.) Then we would have the same situation we have today with all the consumers being forced to have rules - "If it is this type of project, look for the static information. If it is this type of project, do something else to get it". So if we go with the "both" approach that seems it would solve both scenarios. Consumers who don't want to know about these rules (like
No. The full build would depend on this target. Just invoking just this target wouldn't trigger the full build. |
@davkean @BillHiebert - What static properties should we go with for the "run" information? Should we go with the existing: <StartProgram>dotnet.exe</StartProgram>
<StartArguments>exec $(OutDir)$(TargetName).dll</StartArguments>
<StartWorkingDirectory>$(OutDir)</StartWorkingDirectory> set of properties? Or should we invent a new set of properties that potentially take their value from these? |
Moving the CI targets to the CI server
Today GetRunInformation is a Target that returns the information necessary to "run" the project/app. This is intended to be a common place for VS, the CLI, and any other development environment to obtain this information, without having to code the logic in each development environment.
However, in order to get this information, that requires invoking the target (basically executing a 'design-time build').
We should reconsider this approach. A few options that have been raised:
obj
directory that contains this information. VS and the CLI could read this file.obj
directory is. Which means there are two places that need to be interpreted, instead of just one.Another issue that needs to be resolved is "which dotnet.exe host should be used when invoking .NET Core apps?"
One option here is the “host development environment” pass in what it thinks the path to the host should be. So the CLI can pass the path to the “dotnet.exe” that was invoked for “dotnet run”. And VS can pass in a specific host path, if it wants. If no host path is specified, then we read it from the $(PATH) environment variable. We could/should also have properties that the developer can set in their .csproj to use a specific host as well.
/cc @BillHiebert @piotrpMSFT @davkean @srivatsn
The text was updated successfully, but these errors were encountered: