Update required software and developer workflow #30919

ViktorHofer · 2018-07-09T17:23:49Z

ViktorHofer · 2018-07-09T17:38:55Z

Documentation/coding-guidelines/project-guidelines.md

 ```

 ## Updating Configurations

 We have a build task that you can run to automatically update all the projects with the above boilerplate as well as updating all the solution files for the libraries. Whenever you change the list of configurations for a project you can regenerate all these for the entire repo by running:

 ```
-msbuild build.proj /t:UpdateVSConfigurations
+dotnet msbuild build.proj /t:UpdateVSConfigurations


@eerhardt is the UpdateVSConfigurations buildtools task updated to respect SDK project configurations?

Yes, he updated it in: dotnet/buildtools@94f18eb

ViktorHofer · 2018-07-09T17:41:30Z

From Eric:

As of right now, you can’t just call dotnet msbuild on our projects unless you either:

Install the “3.0.100-preview1-009019+” SDK in Program Files
Put “corefx\Tools\dotnetcli” on your $PATH_

--
@eerhardt I was able to successfully build individual projects with dotnet msbuild with .NET Core SDK 2.1.1 installed. Does the mentioned limitation only apply to some projects?

eerhardt · 2018-07-09T20:07:19Z

Does the mentioned limitation only apply to some projects?

Yes. Specifically what fails is the "partial facade" generation. Try building src\System.Collections\src with the 2.1.301 .NET Core SDK, and you should see a failure like:

F:\corefx\Tools\partialfacades.task.targets(64,5): error : Specified method is not supported. [F:\corefx\src\System.Diagnostics.Debug\src\System.Diagnostics.Debug.csproj]
  System.Private.Uri -> F:\corefx\bin\Windows_NT.AnyCPU.Debug\System.Private.Uri\netcoreapp\System.Private.Uri.dll
F:\corefx\Tools\partialfacades.task.targets(64,5): error : Specified method is not supported. [F:\corefx\src\System.Runtime\src\System.Runtime.csproj]

The reason for the failure is because the Microsoft.DiaSymReader.Native assembly that ships in the .NET Core runtime is older than what we need. This is why I needed to move the DotnetCLIVersion.txt up to 3.0.100-preview1-009019. This will be fixed in 2.1.3 by dotnet/core-setup#4292. When we have a 2.1.3 build out, I will move the DotnetCLIVersion.txt back down to that build number.

eerhardt · 2018-07-09T20:22:27Z

Documentation/coding-guidelines/project-guidelines.md

-`<PropertyGroup Condition="'$(Configuration)|$(Platform)' == '$(OSGroup)-$(TargetGroup)-$(ConfigurationGroup)|$(Platform)'">`
-
- Note that the majority of managed projects, currently all in corefx, $(Platform) is overridden to be AnyCPU.
+`<Configurations>netcoreapp-Unix-Debug;netcoreapp-Unix-Release;netcoreapp-Windows_NT-Debug;netcoreapp-Windows_NT-Release;uap-Windows_NT-Debug;uap-Windows_NT-Release;uapaot-Windows_NT-Debug;uapaot-Windows_NT-Release</Configurations>`


I would keep the $(TargetGroup)-$(OSGroup)-$(ConfigurationGroup) placeholders in this line. This one describes the pattern. The next 2 are concrete examples.

eerhardt · 2018-07-09T20:24:23Z

Documentation/debugging/debugging-packages.md

 * Generate a flat project file (out.pp):
-`msbuild my.csproj /pp:out.pp`
+`dotnet msbuild my.csproj /pp:out.pp`


While we are in here, I think it would make sense to also list /bl and http://msbuildlog.com/. This is pretty much exclusively how I debug MSBuild now days.

nice idea, that's definitely worth it!

eerhardt · 2018-07-09T20:26:00Z

Documentation/debugging/debugging-packages.md

@@ -18,7 +18,7 @@ In general, always build the .builds file instead of any of the csproj to ensure

 Assuming the current directory is `\src\contractname\`:

-1. Build the `\ref` folder: `msbuild /t:rebuild contractname.builds` 
+1. Build the `\ref` folder: `dotnet msbuild /t:rebuild contractname.builds` 


@weshaggard - do we still have these contractname.builds files?

No we don't have the builds files any longer. Today if someone wants to build all configurations they do /t:BuildAllConfigurations or (/t:BuildAll) on the csproj for what they want to build.

eerhardt · 2018-07-09T20:27:55Z

Documentation/project-docs/developer-guide.md

 ```

-**Note:** If building in a non-Windows environment, call `<repo-root>/Tools/msbuild.sh` instead of just `msbuild`.


Maybe this should stay in and instead point to

call <repo-root>/Tools/dotnetcli/dotnet msbuild

I think this might be a good place to mention msbuild is a placeholder for dotnet msbuild or msbuild depending on whether dotnet or msbuild is on your path.

eerhardt

Thanks for taking care of this @ViktorHofer !

Looks good. Just some minor comments.

weshaggard · 2018-07-09T21:01:11Z

Documentation/building/code-coverage.md

@@ -56,16 +56,16 @@ This will do the build and testing as with the normal ```build```, but it will r

 You can also build and test with code coverage for a particular test project rather than for the whole repo.  Normally to build and test a particular test suite, from the same directory as that test suite's .csproj, you'd run:

-    msbuild /t:BuildAndTest
+    dotnet msbuild /t:BuildAndTest


The plan is to keep both msbuild and dotnet msbuild working so I wonder if we should simply make a note of that somewhere and just reference msbuild in the instructions.

Question, why should we continue advertising the Desktop msbuild if using the dotnet msbuild is (finally) possible in corefx (from a feature perspective). I don't see any compelling reason to stick with the Desktop flavor. We now already require the .NET Core SDK to be installed which means in 99% of the cases dotnet will be in the path.

That is a fair question. Last I checked even if you called dotnet msbuild on a windows machine with full msbuild it would use the full version so I think we are going to need to keep our builds working for both and in such a case we should allow folks to use either. In the past I know the performance was much better with full msbuild over core msbuild but I'm not sure if that is still the case or not, but another potential reason.

In the past I know the performance was much better with full msbuild over core msbuild but I'm not sure if that is still the case or not, but another potential reason.

I discussed that with @eerhardt offline and when he switched building from the project root with dotnet msbuild he measured performance and it was equal: #30675 (comment). I agree that the dotnet msbuild was slower in the past but I believe it's now on par with the Desktop flavor.

Last I checked even if you called dotnet msbuild on a windows machine with full msbuild it would use the full version

That's interesting. Would be great if we could find out if that's still the case. I just did a quick invocation of dotnet msbuild:

C:\git\corefx\src\System.Text.RegularExpressions>dotnet msbuild /t:Rebuild Microsoft (R) Build Engine version 15.7.179.6572 for .NET Core Copyright (C) Microsoft Corporation. All rights reserved.

and the Task Manager shows .NET Core Host. I believe it's not using the Desktop msbuild anymore underneath but we should verify.

While I thought dotnet msbuild would use the full msbuild your correct that doesn't seem to be the case from some quick testing. However the full msbuild is still used when building from VS, so we will need to maintain our build infrastructure to work with both. So it is really just a matter of which do we advertise in our docs. I'm OK advertising the dotnet msbuild and just adding a note that says you can also call msbuild on our projects.

Also, I'd still be against mandatory pinning for local builds, but my objection wouldn't be as visceral.

I think official builds should be fully repeatable, but freedom to bring my own tools for unofficial, local work is valuable.

but freedom to bring my own tools

This is also SUPER important for source-build and platform boot-strapping purposes. It is great that official builds mandate that this specific set of tools MUST be used. But we also need a way where I can tell this build "this is the toolset to use".

That way we can bootstrap source-build by building once using a pre-built toolset. Then discarding the pre-built toolset, and turning around and re-building using the toolset I just built from source. That's a requirement in most distros that I've seen.

And for platform boot-strapping, it is obvious that I can't use some already built version of the SDK to build on "PlatformX", when there is no existing version that can run on "PlatformX".

Wow that escalated quickly. I recommend creating an issue to discuss updating the SDK via maestro PRs more frequently further and keep the discussion here streamlined on how to invoke the same SDK for both root and individual project builds. Ideally with dotnet build to be consistent as @stephentoub also noted.

I guess the right approach for now is to modify the path to /Tools/dotnetcli/. I'll update the docs and try it myself locally to check if I run into any errors.

@nguerrera if you have better ideas now would be a good time to voice them in the arcade repo

I will do so.

but there will always be tension there as we cannot dogfood the latest sdk build due to other requirements such as source-build.

This actually makes my point much more concrete. We have external reasons to ensure that we can build with some particular toolset, but those shouldn't prevent local dogfooding with newer bits. CI can validate that I haven't taken a dependency on something too new without cutting off my ability to dogfood the new stuff.

But we also need a way where I can tell this build "this is the toolset to use"

Yes, and some one-off gesture shouldn't be needed to dogfood toolsets locally. Source-build will do what it needs to do, but nobody will bother opting in to dogfooding if it doesn't happen naturally/automatically IMHO.

dotnet/arcade#320

weshaggard · 2018-07-09T21:03:38Z

Documentation/building/windows-instructions.md

@@ -103,13 +93,8 @@ For advanced debugging using WinDBG see [Debugging CoreFX on Windows](https://gi

 ### Notes
 * At any given time, the corefx repo might be configured to use a more recent compiler than
-the one used by the most recent Visual Studio IDE release.  This means the corefx codebase might
+the one used by the installed .NET Core SDK. This means the corefx codebase might


I think it is worth adding a pointer to the SDK we use to build so folks can know which version they need. For now that is just to look at the DotnetCLIVersion.txt file but will change to be in global.json once we finish our engineering transition.

tannergooding · 2018-07-09T21:28:16Z

Documentation/building/code-coverage.md

@@ -76,9 +76,9 @@ And then once the run completes:
 Some of the libraries for which contracts and tests live in the corefx repo are actually fully or partially implemented in the core runtime library in another repo, e.g. the implementation that backs the System.Runtime contract is in System.Private.CoreLib.dll in either the coreclr or corert repo. To run coverage reports for these projects, you need to build System.Private.CoreLib locally from the coreclr repo. To get coverage of System.Private.CoreLib while running the tests for a particular library:

 1. Follow the steps outlined at [Testing with Private CoreClr Bits](https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md#testing-with-private-coreclr-bits).  Make sure to include the optional steps listed as being required for code coverage.
-2. Add /p:CodeCoverageAssemblies="System.Private.CoreLib" to the previously discussed msbuild command, e.g. msbuild /t:BuildAndTest /p:Coverage=true /p:CodeCoverageAssemblies="System.Private.CoreLib"
+2. Add /p:CodeCoverageAssemblies="System.Private.CoreLib" to the previously discussed dotnet msbuild command, e.g. dotnet msbuild /t:BuildAndTest /p:Coverage=true /p:CodeCoverageAssemblies="System.Private.CoreLib"

 Note that you will also want to copy the System.Private.CoreLib.pdb along with the System.Private.CoreLib.dll.  As of 10/2017 this PDB must be a windows PDB (Hopefully by early 2018 OpenCOver will directly support portable PDBs.  


Is this comment still relevant? It is now mid 2018 and the issues I can find on the CodeCov repo indicate this may be supported now....

Actually, looks like CodeCov still needs some work: OpenCover/opencover#595

cc: @pjanotti can answer that as he has been taking a look lately into code coverage.

OpenCoverage still only works with WindowsPDB, there is a PR on its repo to update its version of Mono.Cecil to one that supports portable PDBs but it was not merged yet. The comment about having to build System.Private.CoreLib locally only applies if one wants to have "go to source" in coverage report, if one just wants the numbers it is fine to use the downloaded one.

ok thanks. I'll leave it as it is for now.

ViktorHofer · 2018-07-09T23:29:59Z

@eerhardt I will probably halt this PR until the 2.1.3 SDK is released.

weshaggard

LGTM - Thanks @ViktorHofer for updating.

safern

LGTM.

karelz · 2018-08-15T15:41:04Z

@ViktorHofer what is status of this PR? There was no activity for last 4 weeks. Thanks!

ViktorHofer · 2018-08-15T15:44:33Z

I'm waiting because of this: #30919 (comment). When 2.1.3 is out I will merge this PR.

karelz · 2018-08-23T20:56:48Z

@ViktorHofer 2.1.3 shipped. Can we merge now?

ViktorHofer · 2018-08-23T21:02:18Z

Yes, I'll rebase and update the docs now.

* Update required software and developer workflow * Address PR feedback * Fix debugging packages instructions for builds * Add desktop vs core msbuild disclaimer * Delete obsolete file Commit migrated from dotnet/corefx@d9193a1

ViktorHofer added the documentation Documentation bug or enhancement, does not impact product or test code label Jul 9, 2018

ViktorHofer self-assigned this Jul 9, 2018

ViktorHofer requested review from danmoseley and eerhardt July 9, 2018 17:23

ViktorHofer force-pushed the DocsUpdate branch from 829f85b to 498614d Compare July 9, 2018 17:23

ViktorHofer commented Jul 9, 2018

View reviewed changes

ViktorHofer force-pushed the DocsUpdate branch from fb279be to 34b515e Compare July 9, 2018 17:51

eerhardt reviewed Jul 9, 2018

View reviewed changes

eerhardt approved these changes Jul 9, 2018

View reviewed changes

weshaggard reviewed Jul 9, 2018

View reviewed changes

tannergooding reviewed Jul 9, 2018

View reviewed changes

tannergooding approved these changes Jul 9, 2018

View reviewed changes

weshaggard approved these changes Jul 9, 2018

View reviewed changes

ViktorHofer added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 10, 2018

safern approved these changes Jul 10, 2018

View reviewed changes

nguerrera mentioned this pull request Jul 12, 2018

Reconsider requiring global.json to specify the SDK version dotnet/arcade#320

Closed

karelz added this to the 3.0 milestone Jul 19, 2018

karelz added the area-Infrastructure-libraries label Aug 15, 2018

karelz added blocked Issue/PR is blocked on something - see comments and removed * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) labels Aug 15, 2018

ViktorHofer added 4 commits August 23, 2018 23:16

Update required software and developer workflow

bb51f3b

Address PR feedback

4d4ec68

Fix debugging packages instructions for builds

7a7dd0b

Add desktop vs core msbuild disclaimer

ffcf9e9

ViktorHofer force-pushed the DocsUpdate branch from df5684b to e775633 Compare August 23, 2018 21:17

ViktorHofer removed the blocked Issue/PR is blocked on something - see comments label Aug 23, 2018

Delete obsolete file

8b41363

ViktorHofer force-pushed the DocsUpdate branch from e775633 to 8b41363 Compare August 23, 2018 21:19

ViktorHofer merged commit d9193a1 into dotnet:master Aug 23, 2018

ViktorHofer deleted the DocsUpdate branch August 23, 2018 21:23

jkotas mentioned this pull request Jan 1, 2019

Adding msbuild.sh to the root #34272

Closed

		```

		Note: If building in a non-Windows environment, call `<repo-root>/Tools/msbuild.sh` instead of just `msbuild`.

Update required software and developer workflow #30919

Update required software and developer workflow #30919

Conversation

ViktorHofer commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer commented Jul 9, 2018 • edited Loading

eerhardt commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer commented Jul 9, 2018

weshaggard left a comment

Choose a reason for hiding this comment

safern left a comment

Choose a reason for hiding this comment

karelz commented Aug 15, 2018

ViktorHofer commented Aug 15, 2018

karelz commented Aug 23, 2018

ViktorHofer commented Aug 23, 2018

ViktorHofer commented Jul 9, 2018 •

edited

Loading