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

Migrates to RepoToolset infrastructure #2606

Merged
merged 25 commits into from
Aug 29, 2017
Merged

Migrates to RepoToolset infrastructure #2606

merged 25 commits into from
Aug 29, 2017

Conversation

tmat
Copy link
Member

@tmat tmat commented Jul 14, 2017

Migrates the repo to RepoToolset infrastructure. See https://github.com/dotnet/roslyn-tools/blob/master/docs/RepoToolset.md for overview of the toolset.

Recommended review strategy - please review each commit separately, I tried to partition the change into commits to allow for reasonable diffing.

Project System specifics

Layers

The project system is effectively partitioned into layers "HostAgnostic", "VisualStudio" and "VisualStudioDesigner". Previously various traits of the projects were set based on the value of "layer" property set by each project. With this change the different aspects of these layers are captured in Directory.Build.props and Directory.Build.targets in the above subfolders, which makes it easier to see what project belongs to which layer and what properties are applied to it.

XAML rules and Design Time Targets

The .xaml files as well as their translated versions are copied to artifacts\{configuration}\VSSetup\Rules directory.

Design Time targets files were moved under Microsoft.VisualStudio.ProjectSystem.Managed project and are copied along XAML rules to artifacts\{configuration}\VSSetup\Rules directory, where they are picked up by Microsoft.VisualStudio.ProjectSystem.Managed.CommonFiles VS insertion component project.

Integration tests

Integration tests are run by VSIBuild.cmd. This command downloads Roslyn VSIXes during restore build phase, if not downloaded yet, and deploys them to the ProjectSystem hive. It then builds ProjectSystem.sln and runs xunit on build outputs of all projects whose name ends with .IntegrationTests.

The integration test base abstract class was updated to set environment variables that are read by VS to determine the location of the design targets files.

Willow setup components

Project system builds 2 Willow components from .swr files that are not just wrapping VSIXes: Microsoft.VisualStudio.NetCore.ProjectTemplates.1.x and Microsoft.VisualStudio.ProjectSystem.Managed.CommonFiles. The projects that generate the Willow packages are in setup directory in repo root.

Localization

Most of the files changed (deleted) by this PR are translated resource files and xlf files. Translated resource files are no longer needed in the repo. They are generated by xliff build task from xlf files during build.

@tmat tmat force-pushed the RepoToolset branch 2 times, most recently from 21b4dbd to c8cb4e3 Compare July 14, 2017 18:02
Copy link
Member

@davkean davkean left a comment

Choose a reason for hiding this comment

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

Please don't check this in until I've played with this.

@davkean
Copy link
Member

davkean commented Jul 17, 2017

Can you give me a quick summary on what's going on with this change? It looks like you've moved a bunch of cheese, such as removed build.cmd and replaced with Roslyn's version which I dislike.

@davkean
Copy link
Member

davkean commented Jul 17, 2017

Things I'm not a fan of:

  • Adding lots of files to the root - we've clean on root deliberately.
  • Separating out test.cmd vs build.cmd, our unit tests take 15 - 20 seconds to run we run then all the time.
  • Separating our CiBuild.cmd vs build.cmd, we very deliberately has the same build between all our CI builds and what we ran.,
  • You've moved source files around, please back it out. That has nothing to do with this change - and I'd like to discuss the merits of that in a different PR.

@tmat
Copy link
Member Author

tmat commented Jul 17, 2017

@davkean

Adding lots of files to the root - we've clean on root deliberately.

What's the benefit of having fewer files in the root? I find much more useful to have common verbs (such as "test", "restore", "build"): having them there makes it obvious how to do these common tasks in the repo. That's especially beneficial for people not familiar with the repo and for folks who work with multiple repos, who don't need to relearn different command line arguments to various build scripts.

  • Separating out test.cmd vs build.cmd, our unit tests take 15 - 20 seconds to run we run then all the time.

If you want to build and test you can run build -test. In other repos both build and tests take a while so it makes sense to separate these commands. It's also clear for a newcomer how to run tests when they see test.cmd. Currently, test.cmd doesn't restore and build. I don't feel strongly about that and am open to changing it.

  • Separating our CiBuild.cmd vs build.cmd, we very deliberately has the same build between all our CI builds and what we ran.,

I'm not sure i understand. CIBuild.cmd as proposed in this change is directly invoked from Jenkins and Microbuild with a single parameter: configuration. Running CIBuild locally thus gives you exactly the same behavior as on Jenkins and Microbuild (modulo internal-only steps, such as real signing and IBC). You don't need to remember what parameters to set. Currently in this repo microbuild runs

Build.cmd /release /no-node-reuse /no-deploy-extension /signbuild /copy-artifacts

and Jenkins PR runs

SET VS150COMNTOOLS=%ProgramFiles(x86)%\\Microsoft Visual Studio\\Preview\\Enterprise\\Common7\\Tools\\
SET VSSDK150Install=%ProgramFiles(x86)%\\Microsoft Visual Studio\\Preview\\Enterprise\\VSSDK\\
SET VSSDKInstall=%ProgramFiles(x86)%\\Microsoft Visual Studio\\Preview\\Enterprise\\VSSDK\\

build.cmd /no-node-reuse /no-deploy-extension /${configuration.toLowerCase()}

and Jenkins VSI runs:

echo *** Installing 1.0 CLI ***

@powershell -NoProfile -ExecutionPolicy Bypass -Command "((New-Object System.Net.WebClient).DownloadFile('https://download.microsoft.com/download/B/9/F/B9F1AF57-C14A-4670-9973-CDF47209B5BF/dotnet-dev-win-x64.1.0.4.exe', 'dotnet-dev-win-x64.1.0.4.exe'))"
dotnet-dev-win-x64.1.0.4.exe /install /quiet /norestart /log bin\\cli_install.log

echo *** Build Roslyn Project System ***
SET VS150COMNTOOLS=%ProgramFiles(x86)%\\Microsoft Visual Studio\\Preview\\Enterprise\\Common7\\Tools\\
SET VSSDK150Install=%ProgramFiles(x86)%\\Microsoft Visual Studio\\Preview\\Enterprise\\VSSDK\\
SET VSSDKInstall=%ProgramFiles(x86)%\\Microsoft Visual Studio\\Preview\\Enterprise\\VSSDK\\

build.cmd /no-node-reuse /no-deploy-extension /skiptests /integrationtests /${configuration.toLowerCase()}

With this change both Microbuild and Jenkins PR runs use CIBuild.cmd -configuration Release/Debug and Jenkins integration test runs use VSIBuild.cmd -configuration Release/Debug. Much simpler to understand and reproduce locally, imo.

  • You've moved source files around, please back it out. That has nothing to do with this change - and I'd like to discuss the merits of that in a different PR.

Correct, I moved projects to subfolders of src directory. That's intentional to allow usage of msbuild's hierarchical build configuration via Directory.Build.props and Directory.Build.targets. The settings in these files apply to the projects located in the containing directory and below. The project system is effectively partitioned into layers "HostAgnostic", "VisualStudio" and "VisualStudioDesigner". Previously various traits of the projects were set based on the value of "layer" property set by each project. With this change the different aspects of these layers are captured in Directory.Build.props and Directory.Build.targets in the above subfolders, which makes it easier to see what project belongs to which layer and what properties are applied to it.

@tmat
Copy link
Member Author

tmat commented Jul 18, 2017

@jmarolf Ready for local testing. Try it out please.

@jmarolf
Copy link
Contributor

jmarolf commented Jul 18, 2017

@tmat cool thanks. @dotnet/project-system I would also encourage everyone to sync this branch and see if anything feels out of the ordinary.

@tmat
Copy link
Member Author

tmat commented Jul 18, 2017

btw, probably good idea to git clean -xdf before you do.

@tmat
Copy link
Member Author

tmat commented Jul 18, 2017

Note that you'll need to run restore before opening ProjectSystem.sln.

@davkean
Copy link
Member

davkean commented Jul 18, 2017

Why do you need to run restore.cmd before opening projectsystem.sln? We've never needed that. Why do we even need Restore.cmd - before it was implicit. Same with tests.

@tmat
Copy link
Member Author

tmat commented Jul 18, 2017

@davkean Because the common .targets and .props files are now in a nuget package (RepoToolset package). Dotnet SDK doesn't support custom SDKs yet. Once it does an explicit Restore step won't be needed.

@davkean
Copy link
Member

davkean commented Jul 18, 2017

@tmat I'd prefer a model where we don't need to do that, as this is going to introduce a lot of friction that is not going to match the benefit we get from this. Can we please instead have a source drop model? Or figure out a way where we can put these in packages and have them restored correctly?

Once it does an explicit Restore step won't be needed.

You're going to have to update it every time we take a new toolset, yes?

@tmat
Copy link
Member Author

tmat commented Jul 18, 2017

@davkean I'll look into it, perhaps I can figure out something not too complex. A source drop might work. Having to restore doesn't seem to be much of friction for Roslyn though.

@davkean
Copy link
Member

davkean commented Jul 18, 2017

That's because they need to run Restore.cmd anyway, we've never needed an explicit restore step - either build.cmd did it, or VS auto restore kicked in.

This entire repro's design for build/test/open/F5 was based on my experience with Roslyn and how horrible that was. I just went back last week and it's still not very good. I do not want to regress my day to day for consistency with Roslyn.

@davkean
Copy link
Member

davkean commented Jul 18, 2017

I think we need to remember that Roslyn is a huge repro. Based on that, having commands that cut down how long things take/run and to only run for areas your team owns makes sense. Project System is tiny in comparison, I'd prefer to restore, build, run all unit tests in single step for all this repro. I spent a non-trivial amount of time on build.cmd to get it to a point that I was happy with it, I do not feel that your replacement, if based on Roslyn, is on par with it.

@tannergooding
Copy link
Member

I think that, although project-system had some niceties, overall its infrastructure has been much poorer and was not setup to continue working as the repository grew. That is, the infrastructure we had was hacked out from Roslyn and made to be more expedient for a small repository. This also means that are infrastructure has been a decaying snapshot of the Roslyn infrastructure state at the time we cloned it. We still have workarounds/hacks that were only relevant back in VS2015 and pre-Willow and have gotten next to none of the improvements made (batch signing, microbuild validation, build/perf/determinism correctness validation, etc).

The repo-toolset is not doing exactly what Roslyn is doing either, it is more of a cleaned-up and generalized version of its infrastructure and, while not perfect, there has been a lot of time put in making sure that it is useable for both small and big repositories and that it will continue working as a small repository grows. It is designed to make all of our repositories consistent, useable, and easily upgradeable when new infrastructure improvements come around and should be useable by Roslyn as well in the long run.

Overall, consistency across our repositories, IMO, is much more important than making some tasks easier to do on a small repository by default. A public contributor, a long-time dev, or a new hire should be able to come into any dotnet repo (or, at the very least, any roslyn repo) and start working. How to do basic tasks such as restoring, building, testing, emulating any particular CI job locally, locating source code, etc... should all be straightforward and familiar across our repos (learn it once and you can navigate or work with any of our codebases on a basic level). That is simply not possible today as we have at least 4 different models, each with various user's flavor on how they think infrastructure should be done (depending on who built the repository).

That being said, if we determine that there are some developer flows that are desired (such as building and testing in one go), then it should be possible to add an explicit switch. If that still isn't sufficient, dev's can add an alias on their local box to suit their needs (I have several setup to expedite various tasks and being able to share those aliases across repositories is also useful).

There will be initial quirks and things we find that could be improved (such as not having to explicitly restore first, which is a regression), but we should work towards them by improving this common/core infrastructure and by providing a way to do this in our own tools/sdks if we find that it can't be achieved at present.

@basoundr
Copy link
Contributor

@tmat Does the repotoolset have the ability to publish to myget? I was looking at the Analyzer repo and I dont see the myget publishing setup either in build process or in the microbuild process. I could not find it in this change as well

@panopticoncentral
Copy link
Contributor

It doesn't sound like anyone's arguing against consistency between the repos in terms of infrastructure. That seems to be pretty clearly a win.

The biggest point of disagreement seems to be about splitting build into restore/build/test. Given that those are files that are actually checked in to our repo, there isn't any issue with that about sharing infrastructure with other repos. It's purely about how we manage things in our repo. Since all the operations are pretty lightweight, I would also tend towards keeping a single command that does everything. I think the LUT repo does the same thing, then they have switches like "/skipRestore" and "/skipTest" to skip things you might not want. If you want to have the separate commands, you can create aliases pretty easily.

@tannergooding
Copy link
Member

I think the LUT repo does the same thing, then they have switches like "/skipRestore" and "/skipTest" to skip things you might not want.

I wrote the LUT infrastructure and similarly did so in order to help make it better for a smaller repo 😄.

Both the opt-out model that LUT uses and the opt-in model that the repo-toolset uses are good, because it allows you to modularize the build. The biggest difference being whether it is better for a small repo (opt-out, since the entire process is cheap) or better for a large repo (opt-in, since each stage can be very expensive).

However, realistically, the opt-out model is only limited by how expensive we make each step (if we can determine the build step isn't needed and can quickly skip it, opt-out becomes reasonable for large repos as well).

@tmat
Copy link
Member Author

tmat commented Jul 18, 2017

@basoundr Yes, myget publishing is done by a microbuild task. Usually, each repo publishes into a separate feed. E.g. analyzers are now published here: https://dotnet.myget.org/Gallery/roslyn-analyzers

@tmat
Copy link
Member Author

tmat commented Jul 18, 2017

To be clear, this PR has a command that does it all: CIBuild.cmd - it restores, builds, runs tests, tests signing and creates packages and VSIXes. You can definitely use it to validate local changes.

As @tannergooding pointed out you can easily come up with shortcuts that suite your style. If you take a look at what the .cmd files are doing you'll see it's very easy to put together any combination of builds steps you prefer.

For example,
CIBuild.cmd:

powershell -ExecutionPolicy ByPass %~dp0build\Build.ps1 -restore -build -test -sign -pack -ci %*

Build.cmd:

powershell -ExecutionPolicy ByPass %~dp0build\Build.ps1 -restore -build -deploy %*

Note also you can pass the switches on command line. So to restore, build, deploy VSIXes and test you can type:

> build -test

If you want to pack as well then:

> build -test -pack

If you want to set msbuild property you can do that too:

> build /p:Property=Value

Arguments that don't match powershell parameters are passed thru to msbuild.

@tmat tmat force-pushed the RepoToolset branch 4 times, most recently from 925f367 to ca0dda7 Compare July 18, 2017 21:31
@tmat tmat force-pushed the RepoToolset branch 3 times, most recently from 9e5863e to 11281a8 Compare August 29, 2017 20:35
@tmat tmat merged commit 403bdfa into dotnet:master Aug 29, 2017
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.