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 to fmt 10.1.1, fast_double_parser 0.7.0 #6074

Merged
merged 11 commits into from
Sep 12, 2023
Merged

Conversation

jameslamb
Copy link
Collaborator

Contributes to #3763.
Contributes to #5691.

Proposes updated the projects vendored in here as git submodules to their latest versions:

How I did that

# fast_double_parser
cd ./external_libs/fast_double_parser
git fetch origin v0.7.0
git checkout v0.7.0
cd ../../
git add ./external_libs/fast_double_parser

# fmt
cd ./external_libs/fmt
git fetch origin 10.1.1
git checkout 10.1.1
cd ../../
git add ./external_libs/fmt

Benefits of this change

Pulls in bugfixes, security patches, performance improvements.

I'm hoping it'll help with R 4.3 support via changes like this:

CMAKE_CXX_FLAGS
"${CMAKE_CXX_FLAGS} -Wno-stringop-overflow"
)
endif()
Copy link
Collaborator Author

@jameslamb jameslamb Sep 4, 2023

Choose a reason for hiding this comment

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

This is used to suppress the following warning that shows up only with MinGW (gcc 10):

[ 60%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/network/linkers_mpi.cpp.obj

d:\a\lightgbm\lightgbm\lightgbm.rcheck\00_pkg_src\lightgbm\src\external_libs\fmt\include\fmt\format.h: In function 'void fmt::v10::detail::format_hexfloat(Float, int, fmt::v10::detail::float_specs, fmt::v10::detail::buffer<char>&) [with Float = double; typename std::enable_if<(! std::integral_constant<bool, (std::numeric_limits<_Tp>::digits == 106)>::value), int>::type <anonymous> = 0]':
d:\a\lightgbm\lightgbm\lightgbm.rcheck\00_pkg_src\lightgbm\src\external_libs\fmt\include\fmt\format.h:1370:8: note: at offset -2 to object 'buffer' with size 10 declared here
 1370 |   Char buffer[digits10<UInt>() + 1] = {};
      |        ^~~~~~

(example build link)

Per fmtlib/fmt#1810 (comment), this was a false positive and a bug in gcc 10 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95353). Seems that maybe it wasn't backported in MinGW as well?

I saw the same error in only the MinGW + R 4.3 + Rtools43 build on #6061.

@jameslamb
Copy link
Collaborator Author

jameslamb commented Sep 4, 2023

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/6068766481

Status: success ✔️.

@jameslamb jameslamb changed the title WIP: update to fmt 10.1.1, fast_double_parser 0.7.0 update to fmt 10.1.1, fast_double_parser 0.7.0 Sep 4, 2023
@jameslamb jameslamb marked this pull request as ready for review September 4, 2023 14:39
@jameslamb
Copy link
Collaborator Author

@guolinke @shiyu1994 could I get help with a review on this?

In addition to the bugfixes and performance improvements in these new versions of these libraries, I think this PR will help with fully testing the R package against R 4.3 and with efforts like #5981 to make the project compatible with C++20.

@jameslamb
Copy link
Collaborator Author

Thanks so much @guolinke !

@jameslamb jameslamb merged commit 921479b into master Sep 12, 2023
39 checks passed
@jameslamb jameslamb deleted the update-dependencies branch September 12, 2023 18:40
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants