-
Notifications
You must be signed in to change notification settings - Fork 4.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
Update documentation for Update 1 #7024
Update documentation for Update 1 #7024
Conversation
Annoyingly there is no way to do a pull request for the wiki, so I've made a PR at jasonmalinowski/roslyn-wiki#1 which you can comment on there. |
|
||
To debug through tests, you can right click the test project that contains your | ||
tests and choose **Set as Startup Project**. Then press F5. This will run the | ||
tests under the command line runner. Some members of the team have been |
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.
In my experience this doesn't work right now, because the path to the nuget package directory isn't rooted. Can we fix that?
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.
Fixed in #7063.
👍, though @dotnet/roslyn-infrastructure should look at the linux test failure to make sure it's tracked. |
retest prtest/lin/dbg/unit32 please |
b78e909
to
68d4a96
Compare
68d4a96
to
8afb70c
Compare
Thanks for fixing. LGTM |
1. Clone https://github.com/dotnet/roslyn | ||
2. Run the "Developer Command Prompt for VS2015" from your start menu. | ||
3. Run `Restore.cmd` in the command prompt to restore NuGet packages. | ||
4. Due to [Issue #5876](https://github.com/dotnet/roslyn/issues/5876), you should build on the command line before opening in Visual Studio. Run `msbuild /v:m /m Roslyn.sln` |
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.
It is unclear that whether this command does bootstrap, if someone want to make language changes to the compiler and want to use them in projects like Features.
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.
It doesn't; it's just working around a bug where we might get some spurious errors in the IDE if we don't do this. Make sense now, or should I reword it?
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.
OK, so will building inside IDE use the bootstraped compiler? Please consider adding this info when the workaround can be removed, or one have to read cibuild scripts to find out.
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.
The compiler used to build Roslyn is the "toolset compiler" which comes from a NuGet package. If you F5 within VS (or run the dev hive) then that builds with the compiler that you just built. Makes sense?
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.
It makes sense to build the compiler projects of Roslyn using the toolset compiler, then the newly built compiler should take over and build everything. This is what I observed from cibuild output, I think building inside IDE ideally should do the same, but I doubt if it's possible.
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'm not sure if you interpreted "everything" in my last comment:
then the newly built compiler should take over and build everything
as everything including projects outside of Roslyn, no, I meant just everything inside Roslyn(solution/folder) within that particular build session. Does not affect other projects outside.
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.
The problem with switching the compiler version is there's no good way to do that inside Visual Studio. We want to have a reasonable expectation that if I open Roslyn.sln and hit build that it does the same thing as MSBuild on the command line.
Talking with @agocke, his response was if you do want to build the compiler with the compiler like cibuild, then you can just run cibuild.cmd and get that same experience. I agree that's not discoverable; @agocke or @jaredpar do we have better documents somewhere?
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.
Another option is to just use the same hook we use in cibuild.cmd to take over the compiler. CIBuild essentially does the following:
- Build toolset.sln
- Copy
Binaries\Debug
to a new directoryBinaries\Bootstrap
- Run msbuild with
/p:BootstrapBuildPath=Binaries\Bootstrap
The BootstrapBuildPath
hook isn't specific to any path. You could point it to any directory on your machine so long as it had csc and vbc inside it.
I agree that's not discoverable
It's not discoverable because it generally isn't deemed valuable to individual developers. Even on the compiler team we use the toolset compiler for the vast majority of our development. The reason for cibuild using the freshly built compiler is just a sanity check that we can always compile our repo with our repo.
This is a valuable test to have but the breaks it catches are rather rare. Typically we get about 1 per month on the compiler team. The decreased throughput of the compiler when using bootstrap builds isn't worth catching a bug that you'd introduce on average once a year. Hence we do it only in our build lab.
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.
Again let me clarify, I'm suggesting documenting.
@jasonmalinowski 1. Yes, I agree; 2. Yes, that's what I've been doing
@jaredpar 1. I remember this BootrapBuildPath property was documented in a section like "Using the OSS built compiler", but not sure if it also mentioned toolset.sln; 2. Yes, but worth doing locally before submitting pr with compiler changes, or would probably cause unnecessary trouble to the infrastructure team like I did in #6508
Well, thanks for all these info of the discussion, I think it's pretty clear to me, maybe I can try putting this into a new section tomorrow.
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.
As far as the trouble for the infrastructure team; eh, it happens. Right now we're being very strict about filing bugs in PR failures if it's not immediately apparent that the PR is related to that. It looks like we were a bit over-aggressive in that case, but don't worry about that.
|
||
# Getting the Code | ||
|
||
1. Clone https://github.com/dotnet/roslyn |
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.
Fork and clone? Do we discuss the proper process for this somewhere?
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.
Perhaps? I'm not sure I want to include "how to use GitHub" in our instructions, versus assuming that as a base.
My deadline to get this merged in is in three minutes, so I'm going to merge this now, and address the (excellent) feedback from today this afternoon. |
project as your startup project to ensure the right things are built and | ||
deployed. | ||
|
||
- **VisualStudioSetup**: this project builds Roslyn.VisualStudio.Setup.vsix. It |
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.
Are there dependencies between these? Does building VisualStudioSetup guarantee the CompilerExtension is redeployed if it has changes?
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.
It does not. That's why I'm hedging with "you should do a full build to make sure everything is primed." I'm undecided whether this is good or bad -- it means we're only redeploying on what you want, but at the same time it might cause problems.
…r-update-1 Update documentation for Update 1
live analysis are synchronized. | ||
- **ExpressionEvaluatorPackage**: this deploys the expression evaluator and | ||
result providers, the components that are used by the debugger to parse and | ||
evaluate C# and VB expressions in the watch window, immediate window, and |
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.
Consider: "Watch window", "Immediate window"
language features, you may wish to consider building both the | ||
CompilerExtension and VisualStudioSetup projects to ensure the real build and | ||
live analysis are synchronized. | ||
- **ExpressionEvaluatorPackage**: this deploys the expression evaluator and |
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.
Should we name the .vsix files for the following three as well?
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.
Done.
Missed the inevitable merge, but 👍 and great job getting all this updated! |
I've made PR #7127 for the feedback I missed the first time around. |
This updates our building and testing instructions for Update 1. I'm moving the content to the main repository so it can be more easily updated as code changes update it, and since it's "reasonable" that different branches might have different instructions. This is no different than the Linux instructions that are already in the repository.
Reviewers: @dotnet/roslyn-infrastructure