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

Upgrade folly to v2023.12.04.00 (from v2022.11.14.00) #7700

Closed
wants to merge 13 commits into from

Conversation

assignUser
Copy link
Collaborator

This version no longer uses deprecated openssl functions. (the actual change is in an earlier version but I think it makes sense to bump as far as possible while we are at it)

Copy link

netlify bot commented Nov 22, 2023

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 333d297
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65b14bd08f0c7d00077cb990

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 22, 2023
@mbasmanova mbasmanova changed the title Bump bundled folly version Upgrade folly to v2023.11.20.00 (from v2022.11.14.00) Nov 22, 2023
@mbasmanova
Copy link
Contributor

@assignUser Jacob, thank you for the upgrade. Would you rebase and resolve conflicts?

@assignUser
Copy link
Collaborator Author

The nodiscard was just added in facebook/folly@b7af40f so let's try a bit earlier version.

@majetideepak
Copy link
Collaborator

Masha and I magically chose v2023.10.30.00 and it worked. Try that if this does not work.

@assignUser
Copy link
Collaborator Author

magically chose

🧙 😀 nice. Yeah will try if this one fails

@majetideepak
Copy link
Collaborator

And don't forget to update the setup scripts as well.

@majetideepak
Copy link
Collaborator

chose v2023.10.30.00 and it worked

This worked only on MacOS though. Linux was not tested.

@assignUser
Copy link
Collaborator Author

hmm @majetideepak any idea what could cause the glog issue?

@majetideepak
Copy link
Collaborator

I found this link gflags/gflags#203
We have to investigate further.

@majetideepak
Copy link
Collaborator

@assignUser I see that both gflags and glog are being built together even though there is a gflags dependency for glog.
This is likely causing the glog issue.

[46/2257] Building CXX object _deps/gflags-build/CMakeFiles/gflags_nothreads_static.dir/src/gflags_completions.cc.o
[47/2257] Building CXX object _deps/glog-build/CMakeFiles/glogbase.dir/src/demangle.cc.o

@assignUser
Copy link
Collaborator Author

Oh yeah that might be it! I'll check it out!

@assignUser
Copy link
Collaborator Author

@majetideepak looks like you were right! But we seem to have some other errors now... I had to add a new warning suppression to the expression fuzzer test (see commit message for the error) but we should probably just fix that.

@majetideepak
Copy link
Collaborator

I am having a hard time trying to understand these warnings.
Where does facebook::velox::test::ExpressionFuzzer::Options::skipFunctions come from?

/velox/velox/expression/tests/ExpressionFuzzerUnitTest.cpp: In lambda function:
/velox/velox/expression/tests/ExpressionFuzzerUnitTest.cpp:107:35: error: missing initializer for member 'facebook::velox::test::ExpressionFuzzer::Options::skipFunctions' [-Werror=missing-field-initializers]
  107 |         {{.maxLevelOfNesting = 5}}};
      |                                   ^
/velox/velox/expression/tests/ExpressionFuzzerUnitTest.cpp: In member function 'virtual void facebook::velox::test::ExpressionFuzzerUnitTest_exprBank_Test::TestBody()':
/velox/velox/expression/tests/ExpressionFuzzerUnitTest.cpp:133:51: error: missing initializer for member 'facebook::velox::test::ExpressionFuzzer::Options::skipFunctions' [-Werror=missing-field-initializers]
  133 |         {{.maxLevelOfNesting = maxLevelOfNesting}}};

@mbasmanova
Copy link
Contributor

Is this relevant?

git show b9367aa14

Author: Laith Sakka <lsakka@meta.com>
Date:   Mon Dec 4 18:01:07 2023 -0800

    Back out "make skipFunctions, onlyFunctions, and specialForms as ExpressionFuzzer options."

    Summary:
    Dont have time to investigate will backout and now and fix later.
    Original commit changeset: c4bb6b830dc0

CC: @kevinwilfong @laithsakka

@majetideepak
Copy link
Collaborator

@mbasmanova thanks for the pointer. That is relevant.
@assignUser rebasing with main should fix the warnings your are seeing.
Do you know how to fix the MacOS build failure related to OpenSSL?

@majetideepak
Copy link
Collaborator

majetideepak commented Dec 5, 2023

I am looking into the linux-build failure

../../velox/exec/tests/utils/QueryAssertions.cpp:1294
Value of: waitForTaskCompletion(task, maxWaitMicros)
  Actual: false
Expected: true

@assignUser
Copy link
Collaborator Author

@majetideepak rebased and add the changes for the spark fuzzer.

@assignUser
Copy link
Collaborator Author

reverting the init change to the fuzzer files and adding -Wno-deprecated-declarations to that entir dir works but we should probably move those over to the new init version. I will create an issue.

@assignUser assignUser linked an issue Dec 6, 2023 that may be closed by this pull request
facebook-github-bot pushed a commit that referenced this pull request Jan 24, 2024
Summary:
fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during #7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes #7896

Pull Request resolved: #7941

Reviewed By: Yuhta

Differential Revision: D52570200

Pulled By: kgpai

fbshipit-source-id: 267840a66d05b03eb25c2c278a694d062770dcea
bump to newest version

Revert "bump to newest version"

This reverts commit 016f862.

use 2023.10.30

use newest folly
this properly suppresses all warnings that happen within folly headers (e.g. openssl)
@assignUser
Copy link
Collaborator Author

@kgpai @majetideepak could one of you look at the test failures and benchmark segfault? These are out of my wheelhouse. This PR was green before the rebase (300+ new commits) on the fmt upgrade (but I tested it with fmt 10 before and it worked fine).

@kgpai
Copy link
Contributor

kgpai commented Jan 24, 2024

@assignUser No worries, I can take a look.

@majetideepak
Copy link
Collaborator

@assignUser For some reason I see folly::Init{&argc, &argv, false}; being used instead of folly::Init init(&argc, &argv);.
We discussed the problem here #7700 (comment)

@assignUser
Copy link
Collaborator Author

Ah good catch I must have dropped the wrong commit during the rebase. I will fix in a bit, maybe that solves the problems already!

@majetideepak
Copy link
Collaborator

majetideepak commented Jan 24, 2024

@assignUser If you can fix the format-check, this should be ready to be merged.

@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@kgpai merged this pull request in 85f0664.

Copy link

Conbench analyzed the 1 benchmark run on commit 85f0664f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Feb 2, 2024
…tor#7700)

Summary:
This version no longer uses deprecated openssl functions. (the actual change is in an earlier version but I think it makes sense to  bump as far as possible while we are at it)

Pull Request resolved: facebookincubator#7700

Reviewed By: Yuhta

Differential Revision: D51966751

Pulled By: kgpai

fbshipit-source-id: 33df50e9c6b92459247b7e578a453a2d24e0d6dc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
5 participants