Skip to content

Conversation

@pdillinger
Copy link
Contributor

@pdillinger pdillinger commented Oct 22, 2025

Summary: Resolving this folly upgrade required fixing the FOLLY_LITE build with header include from the 'fmt' library.

I was close to timing out on fixing USE_FOLLY_LITE and removing it altogether - it could be considered obsolete and/or not worth the maintenance cost.

Follow-up: make the folly build caching more friendly by hashing the relevant makefile parts. Not in this PR because then you wouldn't be able to see what changed in the folly build steps themselves.

Test Plan: CI

Summary: Resolving this folly upgrade required fixing the FOLLY_LITE
build with header include from the 'fmt' library.

I was close to timing out on fixing USE_FOLLY_LITE and removing it
altogether - it could be considered obsolete and/or not worth the
maintenance cost.

Test Plan: CI
@pdillinger pdillinger requested a review from jaykorean October 22, 2025 18:56
@meta-cla meta-cla bot added the CLA Signed label Oct 22, 2025
@meta-codesync
Copy link

meta-codesync bot commented Oct 22, 2025

@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D85268833.

Copy link
Contributor

@jaykorean jaykorean left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this!

Makefile Outdated
perl -pi -e 's/memcpy.&ptr/memcpy((void*)&ptr/' third-party/folly/folly/lang/Exception.cpp
@# const mismatch
perl -pi -e 's/: environ/: (const char**)(environ)/' third-party/folly/folly/Subprocess.cpp
@# NOTE: boost source will be needed for any build including `USE_FOLLY_LITE` builds as those depend on boost headers
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we don't need to fetch fmt here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My local build apparently already had folly built from a full folly build.

And now I'm sure I would hit another CI failure if I just made the fix because of the new folly caching which does not hash the relevant makefile parts, so I'm going to arbitrarily change the folly git hash again :(

@pdillinger
Copy link
Contributor Author

Well, that's concerning:

2025-10-22T19:34:02.2720515Z �[0;33mNote: Google Test filter = FormatLatest/ColumnFamilyTest.BulkAddDrop/0
2025-10-22T19:34:02.2721287Z �[m�[0;32m[==========] �[mRunning 1 test from 1 test case.
2025-10-22T19:34:02.2721872Z �[0;32m[----------] �[mGlobal test environment set-up.
2025-10-22T19:34:02.2722512Z �[0;32m[----------] �[m1 test from FormatLatest/ColumnFamilyTest
2025-10-22T19:34:02.2723215Z �[0;32m[ RUN      ] �[mFormatLatest/ColumnFamilyTest.BulkAddDrop/0
2025-10-22T19:34:02.2723611Z 
2025-10-22T19:34:02.2723617Z 
2025-10-22T19:34:02.2723816Z Assertion failure: hp.second == srcChunk->tag(srcI)
2025-10-22T19:34:02.2724284Z Message: 
2025-10-22T19:34:02.2724791Z File: /__w/rocksdb/rocksdb/third-party/folly/folly/container/detail/F14Table.h
2025-10-22T19:34:02.2725398Z Line: 2356
2025-10-22T19:34:02.2725671Z Function: rehashImpl

@jaykorean
Copy link
Contributor

@pdillinger oh wow. Should we file an issue for folly?

@jaykorean jaykorean self-requested a review October 22, 2025 20:59
@pdillinger
Copy link
Contributor Author

@pdillinger oh wow. Should we file an issue for folly?

It's more likely a rare race or UAF or something in our code, though I haven't been able to reproduce it.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants