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

Update build instructions for Visual Studio 2019 #4394

Closed

Conversation

drlongle
Copy link
Contributor

@drlongle drlongle commented Jan 20, 2023

High Level Overview of Change

Update the build instructions for Visual Studio 2019.

Also fix #3037.

Context of Change

It is helpful to provide build instructions for Visual Studio to make it easier for people to build rippled on Windows. Dependencies are installed using Conan. If you have issues building, it may be helpful to uninstall Visual Studio and use Chocolatey to re-install it.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

BUILD.md Outdated
> **Warning**
> The commands in this document are not meant to be blindly copied and pasted.
> The commands in this document are guidelines which cannot always be successfully copy and pasted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would call them examples, not guidelines.

copy -> copied
or
copy and pasted -> copy-and-pasted

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please break lines at 80 characters. Yes, even Markdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

BUILD.md Outdated
### All Platforms

To build this package, you will need [Python][] (>= 3.7),
[Conan][] (>= 1.52), and [CMake][] (>= 3.16).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not believe Conan 1.52 is sufficient any more. We should recommend the latest anyway given all the recent changes (1.58).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

BUILD.md Outdated
To build this package, you will need [Python][] (>= 3.7),
[Conan][] (>= 1.52), and [CMake][] (>= 3.16).
If you are unfamiliar with Conan,
there is a crash course at the end of this document.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Beginning of the document. Are these changes based off an old version of this document?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

I am not sure what version @justinr1234 based his changes on.

BUILD.md Outdated

You'll need to compile in the C++20 dialect:

```
conan profile update settings.compiler.cppstd=20 default
conan profile update settings.cppstd=20 default
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the wrong command, from an old version of Conan. It was already updated in this document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

Comment on lines +197 to +201
If you've never used Conan before, use autodetect to setup a default profile:

```
conan profile new default --detect
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

This command is already in the crash course. This section assumes the person has installed and configured Python, Conan, and CMake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not seem too bad to repeat that command. However, I'll be happy to remove it if you prefer. Please let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I think it is good to repeat the command. Obviously, there is a balance (we wouldn't want to repeat it 5 times). But this looks ok to me.

BUILD.md Outdated
Comment on lines 241 to 269
Let's start with a couple of examples of common workflows.
The first is for a single-configuration generator (e.g. Unix Makefiles) on
Linux or MacOS:

```
conan export external/rocksdb
mkdir .build
cd .build
conan install .. --output-folder . --build missing --settings build_type=Release
cmake -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake -DCMAKE_BUILD_TYPE=Release ..
cmake --build .
./rippled --unittest
```

The second is for a multi-configuration generator (e.g. Visual Studio) on
Windows:

```
conan export external/rocksdb
mkdir .build
cd .build
conan install .. --output-folder . --build missing --settings build_type=Release --settings compiler.runtime=MT
conan install .. --output-folder . --build missing --settings build_type=Debug --settings compiler.runtime=MTd
cmake -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake ..
cmake --build . --config Release
cmake --build . --config Debug
./Release/rippled --unittest
./Debug/rippled --unittest
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these instructions removed? To what steps do the numbered explanations correspond?

Copy link
Contributor Author

@drlongle drlongle Feb 15, 2023

Choose a reason for hiding this comment

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

@justinr1234 probably removed them by mistake. I put them back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was removed when there wasn’t a separate readme.

As far as these instructions, it shouldn’t have Debug and Release together like that as both myself and @drlongle ran into issues with mixing debug and release builds back to back.

Comment on lines 9 to 14
While there are several options for building `rippled` on Windows, we recommend using
[Chocolatey, Conan, CMake and Visual Studio](README.md#build-using-chocolatey-conan-cmake-and-visual-studio-recommended).
If using this option, you can skip the section [Install Software](README.md#install-software)
because Conan automatically installs all dependencies for `rippled`.


Copy link
Collaborator

Choose a reason for hiding this comment

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

Editing this document reflects a totally different philosophy on how to present build instructions in this repo. I've got a waiting PR to remove this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better to revert all changes for Builds/VisualStudio2019/README.md and add Windows-specific instructions to BUILD.md?

@dangell7
Copy link
Collaborator

bump for .gitignore :)

@intelliot
Copy link
Collaborator

FWIW, I would have been fine with merging #4397 as a "quick fix" while this other PR is undergoing review and revisions.

@intelliot
Copy link
Collaborator

  • we can revert the changes to the Windows part
  • other changes can be made in BUILD.md

oeggert added a commit to oeggert/rippled that referenced this pull request Feb 18, 2023
@intelliot intelliot removed the request for review from ximinez April 6, 2023 23:33
@justinr1234
Copy link
Collaborator

@drlongle can you update conflicts and I’ll do a review?

@thejohnfreeman
Copy link
Collaborator

I'd like to see most of this moved to docs/environment.md, like we agreed in #4433 (reply in thread).

@drlongle
Copy link
Contributor Author

I'd like to see most of this moved to docs/environment.md, like we agreed in #4433 (reply in thread).

Agreed. Unless there is any objection, let's delete this PR and open a new one.

@intelliot
Copy link
Collaborator

@drlongle feel free to close this PR and open a new one, at your convenience.

@drlongle drlongle closed this Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 📋 Backlog
Development

Successfully merging this pull request may close these issues.

IANA-registered port number for XRP Ledger
6 participants