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

Refactor build instructions #4381

Merged
merged 5 commits into from
Mar 22, 2023
Merged

Refactor build instructions #4381

merged 5 commits into from
Mar 22, 2023

Conversation

thejohnfreeman
Copy link
Collaborator

@thejohnfreeman thejohnfreeman commented Jan 6, 2023

This is a blanket removal of the build instructions at Builds/{VisualStudio2019,macos,linux}. Most of these instructions are obsolete, e.g. installing dependencies. There are a few notable exceptions that I want to discuss how to handle.

  • I have not ported documentation of the jemalloc or static options to BUILD.md. I do not believe that jemalloc is used by anyone. I think it was only used by @mtrippled, and that he doesn't use it any more. Please correct me if I'm wrong on that. I'm not aware of anyone who turns the static option OFF (its default is ON), and supporting the OFF value would require us to change conanfile.py anyway, to propagate that choice to the shared option of the affected dependencies. I do not believe this option is appropriate to have in our project anyway. By using Conan, we can let builders choose which dependencies specifically to build and link as shared objects. I prefer that we remove both of these options. Objections?
  • I have not ported documentation on how to install rippled or depend on the xrpl-core library from a separate project. These might be worth preserving (and fixing), but I'm not sure they belong in BUILD.md. Suggestions?

@scottschurr
Copy link
Collaborator

FWIW, I personally do not use the jemalloc or static options, and I believe that static ON is the correct default.

It seems like preserving information on installing rippled would be worthwhile, but I have no opinion on where such documentation should live. Similarly, instructions on how to depend on the xrpl-core library seem useful, but I have no opinion on where those instructions should live.

@ximinez
Copy link
Collaborator

ximinez commented Jan 6, 2023

I suggest that one way to port and preserve the documentation that might still be useful would be move it to a Builds/Outdated.md, Builds/Compatibility.md, Builds/external.md, or something like that. Put a note at the top of that file that the documentation has not been vetted against Conan (and link to BUILD.md), so YMMV.

@mDuo13
Copy link
Collaborator

mDuo13 commented Jan 10, 2023

I have been using -Dstatic=OFF when building on Arch (and Manjaro) because the static version of openssl is not readily available in the repos. I haven't tried building since the switch to Conan, though, so I don't know if that impacts things.

I think if you look at the history, the static=OFF option was added to the build instructions specifically because of Arch & Manjaro users.

@MarkusTeufelberger
Copy link
Collaborator

Well, static=OFF was also included in the hopes of rippled eventually making it into a few more distro repositories over time, static compilation is a non-starter in most cases there.

@thejohnfreeman
Copy link
Collaborator Author

With Conan, static vs shared linkage is selected per-dependency with a Boolean option that every Conan package conventionally calls shared. The conanfile for rippled selects shared=False as the default value for all dependencies. Anyone building rippled can override that choice per-dependency at build time (specifically when they run conan install). This is more flexible for end-users and requires no extra effort from us as maintainers. I prefer to leave the static option undocumented in this PR, and would like to remove it in a future PR.

Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

I don't see any issues with removing these files that haven't already been noted in the PR

@thejohnfreeman
Copy link
Collaborator Author

thejohnfreeman commented Mar 3, 2023

I have added some changes that refactor the build instructions based on the plan I laid out in #4433. The only part missing here from that plan is the dependency instructions, which I plan to add in a separate PR along with the Conan recipe I pitched in #4443.

If these changes are merged, I have a few requests:

@intelliot
Copy link
Collaborator

@thejohnfreeman from your perspective, is this PR ready to merge now?

@thejohnfreeman
Copy link
Collaborator Author

No, the reviews should have been invalidated.

@thejohnfreeman thejohnfreeman requested review from ximinez, intelliot and drlongle and removed request for manojsdoshi March 3, 2023 13:42
@thejohnfreeman thejohnfreeman requested review from justinr1234 and removed request for ximinez and intelliot March 3, 2023 13:43
@thejohnfreeman
Copy link
Collaborator Author

GitHub will not let me request a review from @oeggert for some reason. I cannot even autocomplete a tag for him in my comments.

@thejohnfreeman thejohnfreeman changed the title Remove obsolete build instructions Refactor build instructions Mar 3, 2023
Copy link
Collaborator

@oeggert oeggert left a comment

Choose a reason for hiding this comment

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

Looks good. I'll update #4423 with this new structure in mind.

Copy link
Collaborator

@intelliot intelliot left a comment

Choose a reason for hiding this comment

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

@thejohnfreeman confirmed this is good to merge now. I don't see any risky changes, and any changes/fixes can be easily proposed in a follow-up PR.

@intelliot intelliot merged commit 7745c72 into XRPLF:develop Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants