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

[TRIVIAL] Fix NuDB build error #5061

Merged
merged 9 commits into from
Jul 29, 2024
Merged

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Jul 10, 2024

High Level Overview of Change

Fix a build error that seems to be limited to non-unity builds on Windows (Visual Studio) due to a missing #include in NuDB.

Context of Change

I think the reordering of #includes in the physical redesign PR (#4997) removed a silent dependency from one of the headers in NuDB. The dependency version of NuDB didn't change, as far as I can tell. CI didn't catch the issue because the Windows build uses unity to save time.

The error is:

C:\Users\passp\.conan\data\nudb\2.0.8\_\_\package\5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9\include\nudb/detail/stream.hpp(64,20): error C2039: 'logic_error': is not a member of 'std' [C:\Dev\rippled\upstream\build\cmake\msvc19.OFF\rippled.vcxproj]

std::logic_error requires <stdexcept>, but nudb/detail/stream.hpp does not include it.

My assumption is some other header that had previously been ordered before stream.hpp and included <stdexcept> is now ordered after stream.hpp. I don't think it's worth figuring out which file it is, because it's obviously a problem with NuDB.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Build

Future Tasks

See also: cppalliance/NuDB#100

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.4%. Comparing base (d54151e) to head (76561ad).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5061     +/-   ##
=========================================
- Coverage     71.4%   71.4%   -0.0%     
=========================================
  Files          796     796             
  Lines        67039   67039             
  Branches     10865   10867      +2     
=========================================
- Hits         47841   47833      -8     
- Misses       19198   19206      +8     

see 5 files with indirect coverage changes

Impacted file tree graph

@ximinez
Copy link
Collaborator Author

ximinez commented Jul 11, 2024

Rewrote this to patch the source via Conan.

Copy link
Collaborator

@thejohnfreeman thejohnfreeman left a comment

Choose a reason for hiding this comment

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

Do you want to add an instruction to BUILD.md explaining how to export the right version of nudb from external/nudb?

ximinez added 2 commits July 19, 2024 17:06
* Add instructions to BUILD.md to export correct NuDB version
* Also add instructions to BUILD.md to enable recipe revisions, which
  will cause NuDB to build if necessary
* upstream/develop:
  Add xrpld build option and Conan package test (5052)
BUILD.md Outdated Show resolved Hide resolved
* upstream/develop:
  Update BUILD.md after PR 5052 (5067)
@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jul 25, 2024
ximinez added 2 commits July 25, 2024 17:54
* upstream/develop:
  chore: Add comments to SignerEntries.h (5059)
  chore: Rename two files from Directory* to Dir*: (5058)
@ximinez ximinez merged commit 8fc805d into XRPLF:develop Jul 29, 2024
20 checks passed
@ximinez ximinez deleted the pr/nudb-header branch July 29, 2024 22:14
@ximinez ximinez mentioned this pull request Jul 31, 2024
vlntb pushed a commit to vlntb/rippled that referenced this pull request Aug 23, 2024
* Includes updated instructions in BUILD.md.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Build System Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Perf impact not expected Change is not expected to improve nor harm performance. Trivial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants