Skip to content

Updated ReadMe.md - Use the vcpkg submodule instead of separately cloning #761 #873

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
StephanTLavavej merged 8 commits into from
Aug 22, 2020
Merged

Updated ReadMe.md - Use the vcpkg submodule instead of separately cloning #761 #873

StephanTLavavej merged 8 commits into from
Aug 22, 2020

Conversation

@amit-rastogi
Copy link
Contributor

@amit-rastogi amit-rastogi commented May 31, 2020 •
edited by StephanTLavavej
Loading

Updated the build instructions as part of #761 in ReadMe.md to use the vcpkg submodule present in this STL repository instead of separately cloning. The user can alternatively, use a separate installation of vcpkg.

Both the build steps mentioned below have been updated -

  1. How To Build With The Visual Studio IDE
  2. How To Build With A Native Tools Command Prompt

Fixes #761.

@amit-rastogi
Updated ReadMe.md microsoft#761
0a19a1a 
Updated the build instructions in ReadMe.md to use the vcpkg submodule present in this STL repository. The user can alternatively, use a separate installation of vcpkg. Both the build steps given below have been updated -
1. How To Build With The Visual Studio IDE
2. How To Build With A Native Tools Command Prompt
@amit-rastogi amit-rastogi requested a review from a team as a code owner May 31, 2020 19:45
SuperWig
SuperWig reviewed Jun 1, 2020
View reviewed changes
@amit-rastogi
Updated ReadMe.md with review comments
f49e3b8 
Updated ReadMe.md with the following steps as per review comments-

cd STL
git submodule update --init -- vcpkg/
@CaseyCarter CaseyCarter added the documentation Related to documentation or comments label Jun 3, 2020
StephanTLavavej
StephanTLavavej reviewed Jun 3, 2020
View reviewed changes
@amit-rastogi
Updated ReadMe.md as per review comments
8c0c673 
Updated ReadMe.md as per review comments mainly -
-removed vcpkg submodule init for IDE build step. 
-updated vcpkg init command for Command Prompt build step
-updated example path of vcpkg specified to cmake to use the repository submodule path
SuperWig
SuperWig reviewed Jun 4, 2020
View reviewed changes
@StephanTLavavej StephanTLavavej mentioned this pull request Jun 8, 2020
Closed
@amit-rastogi
Updated ReadMe to incorporate review
588df68 
1. reverted back to using command prompt for cloning the STL repository, init of vcpkg and boost math installation in case of build steps using IDE.
2. added --progress option to submodule update of vcpkg as mentioned in microsoft#805
Chronial
Chronial reviewed Jul 8, 2020
View reviewed changes
@StephanTLavavej StephanTLavavej added the decision needed We need to choose something before working on this label Jul 8, 2020
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jul 8, 2020

Tagging as decision needed because as @Chronial noted, changing the global configuration for vcpkg to point to a repo-local submodule may not be desirable. We'll need to avoid this problem, probably by changing only the command-line instructions to use the submodule.

cbezault
cbezault reviewed Jul 29, 2020
View reviewed changes
@cbezault cbezault removed the decision needed We need to choose something before working on this label Jul 29, 2020
@BillyONeal BillyONeal mentioned this pull request Aug 1, 2020
Merged
@CaseyCarter CaseyCarter removed their assignment Aug 12, 2020
@ghost ghost deleted a comment from msftclas Aug 19, 2020
@cbezault
Update README.md
05ba28a 
After microsoft#1124 was merged the steps to get started were simplified to not need `vcpkg integrate`.
CaseyCarter
CaseyCarter approved these changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

This all looks good - just a minor mechanical tweak that I'll merge.

@StephanTLavavej
Adjust backticks, drop ARM/ARM64 instructions.
6b99aec 
According to https://github.github.com/gfm/#code-spans , we don't need
4 backticks; 2 will work. We can also get the space trimmed away - we
need a single space before and after for that to work. Finally,
capitalize Ctrl.

Drop the mention of adding `boost-math:arm-windows boost-math:arm64-windows`
to the `vcpkg.exe install` command - this requires ARM/ARM64 support to
be installed in VS, it has to be performed as a separate
`vcpkg.exe install` command (because it requires `boost-build:x86-windows`,
installed by the first command), this isn't mentioned in the
command-line instructions, and it is unnecessary for most users.

We can also drop "Assuming you are targeting x86 and x64," so this
exactly matches the command-line phrasing.
@StephanTLavavej StephanTLavavej mentioned this pull request Aug 19, 2020
Closed
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 19, 2020

Thanks! I pushed two changes: a small Markdown cleanup, and removing pre-existing ARM/ARM64 wording that wasn't quite right.

  1. According to https://github.github.com/gfm/#code-spans , we don't need 4 backticks; 2 will work. We can also get the space trimmed away - we need a single space before and after for that to work. Finally, capitalize Ctrl.

  2. Drop the mention of adding boost-math:arm-windows boost-math:arm64-windows to the vcpkg.exe install command - this requires ARM/ARM64 support to be installed in VS, it has to be performed as a separate vcpkg.exe install command (because it requires boost-build:x86-windows, installed by the first command), this isn't mentioned in the command-line instructions, and it is unnecessary for most users. We can also drop "Assuming you are targeting x86 and x64," so this exactly matches the command-line phrasing.

I've filed #1211 to track explaining ARM/ARM64 properly; that shouldn't hold up this PR, which now nicely aligns the IDE and command-line instructions.

@BillyONeal
Copy link
Member

BillyONeal commented Aug 20, 2020

As long as someone actually ran through this from a clean box and checked that it worked that sounds good to me

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Aug 20, 2020

As long as someone actually ran through this from a clean box and checked that it worked that sounds good to me

I ran through everything but the ARM/ARM64 bits that we excised and had success.

@StephanTLavavej StephanTLavavej self-assigned this Aug 21, 2020
@StephanTLavavej StephanTLavavej merged commit a293fad into microsoft:master Aug 22, 2020
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 22, 2020

Thanks for improving the "getting started" experience for new users, and congratulations on your first microsoft/STL commit! 🎉 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@mnatsuhara mnatsuhara Awaiting requested review from mnatsuhara

+4 more reviewers

@Chronial Chronial Chronial left review comments

@SuperWig SuperWig SuperWig left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@cbezault cbezault cbezault approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

documentation Related to documentation or comments

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

README.md: Use the vcpkg submodule instead of separately cloning

8 participants

@amit-rastogi @StephanTLavavej @BillyONeal @CaseyCarter @Chronial @cbezault @SuperWig @mnatsuhara