Skip to content

Spruce up BuildFromSource docs based on feedback #26707

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 9, 2020
Merged

Spruce up BuildFromSource docs based on feedback #26707

merged 7 commits into from
Oct 9, 2020

Conversation

captainsafia
Copy link
Member

A little spruce up of the build docs based on some feedback we've gotten from folks participating in the sprints:

  • Organize the docs to make them read more linearly
  • Move the troubleshooting component into its own section
  • Pull out what dependencies are needed to successfully build

dougbu
dougbu reviewed Oct 8, 2020
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

This is a great improvement❕ Some of my comments and suggestions are nits and the count definitely doesn't reflect how much better the doc looks.

docs/BuildFromSource.md Outdated
> You can do so by running the `Set-ExecutionPolicy -ExecutionPolicy RemoteSigned -Scope CurrentUser` command
> in PowerShell. For more information on execution policies, you can read the [execution policy docs](https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.security/set-executionpolicy).

If you already have Visual Studio installed, you can use the installation script above to ensure that you have the correct components installed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest removing this line. It's redundant given https://github.com/dotnet/aspnetcore/pull/26707/files#diff-a7fae696e4f4a8d72dea5e224d7463bcR45

docs/BuildFromSource.md Outdated

If you already have Visual Studio installed, you can use the installation script above to ensure that you have the correct components installed.

The [global.json](/global.json) and [eng/scripts/vs.json](/eng/scripts/vs.json) provide a description of the components required to support building the repo.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest adding a note that global.json only specifies the minimum requirements to build from the command line i.e. that file ensures the native builds (which use desktop msbuild) can work. Need the additional components from vs.json to build within Visual Studio.

Suggest also mentioning that InstallVisualStudio.ps1 uses vs.json by default.

docs/BuildFromSource.md Outdated
2. Solution files group together projects which are frequently edited at the same time.
3. Can't find a solution that has the projects you care about? Feel free to make a PR to add a new .slnf file.


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra line

uabarahona
uabarahona reviewed Oct 8, 2020
View reviewed changes
docs/BuildFromSource.md Outdated
@@ -224,13 +266,15 @@ On macOS/Linux:
./build.sh --test
```

## Building a subset of the code
> :bulb: If you're working on changes for a particular subset of the project, you might not want to execute the entire test suite. Instead, only run the tests within the subdirectory where changes were made.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does build.cmd have a way to execute a particular subset of test? or do we need to activate and then do dotnet test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, e.g. .\build.cmd -test -projects .\src\Framework\test\Microsoft.AspNetCore.App.UnitTests.csproj. You could add -noBuild to that command line if the build is already completely up-to-date.

Copy link
Contributor

@uabarahona uabarahona Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Awesome! thanks

Pilchie
Pilchie approved these changes Oct 8, 2020
View reviewed changes
Copy link
Member

@Pilchie Pilchie left a comment

Choose a reason for hiding this comment

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

This is great, I have only nits/minor suggestions, but really love the updates here.

Thanks a bunch @captainsafia !

Pilchie
Pilchie reviewed Oct 8, 2020
View reviewed changes
@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Oct 9, 2020
@captainsafia captainsafia merged commit 450e047 into master Oct 9, 2020
@captainsafia captainsafia deleted the safia/update-readme branch October 9, 2020 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pilchie Pilchie Pilchie approved these changes

@dougbu dougbu dougbu left review comments

@uabarahona uabarahona uabarahona left review comments

@wtgodbe wtgodbe Awaiting requested review from wtgodbe

@javiercn javiercn Awaiting requested review from javiercn

@ajaybhargavb ajaybhargavb Awaiting requested review from ajaybhargavb

Assignees
No one assigned
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@captainsafia @Pilchie @dougbu @uabarahona