Skip to content

fbthrift: update Linux gcc dependency#103112

Closed
cho-m wants to merge 1 commit intoHomebrew:masterfrom
cho-m:fbthrift-gcc11
Closed

fbthrift: update Linux gcc dependency#103112
cho-m wants to merge 1 commit intoHomebrew:masterfrom
cho-m:fbthrift-gcc11

Conversation

@cho-m
Copy link
Copy Markdown
Member

@cho-m cho-m commented Jun 7, 2022

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

I think GCC 11 issue is the one mentioned in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104008 as referenced by commit facebook/folly@5e6bef2

It seems to be fixed in GCC 11.3 so trying newer gcc formula to reduce multiple GCC versions in dependency tree (since folly/fizz/... use gcc).

@cho-m cho-m marked this pull request as ready for review June 7, 2022 04:32
@carlocab
Copy link
Copy Markdown
Member

carlocab commented Jun 7, 2022

We can probably also try building watchman with Eden support on Linux so that we have feature-parity (and minimise the OS-conditionals).

@carlocab
Copy link
Copy Markdown
Member

carlocab commented Jun 7, 2022

No, still doesn't build:

  In file included from /tmp/watchman-20220607-31055-19g08zv/watchman-2022.06.06.00/watchman/watcher/eden.cpp:8:
  /home/linuxbrew/.linuxbrew/include/cpptoml.h: In static member function 'static cpptoml::value_traits<T, typename std::enable_if<(((! cpptoml::valid_value_or_string_convertible<T>::value) && (! std::is_floating_point<typename std::decay<_Tp>::type>::value)) && std::is_signed<typename std::decay<_Tp>::type>::value)>::type>::value_type cpptoml::value_traits<T, typename std::enable_if<(((! cpptoml::valid_value_or_string_convertible<T>::value) && (! std::is_floating_point<typename std::decay<_Tp>::type>::value)) && std::is_signed<typename std::decay<_Tp>::type>::value)>::type>::construct(T&&)':
  /home/linuxbrew/.linuxbrew/include/cpptoml.h:344:25: error: 'numeric_limits' is not a member of 'std'
    344 |         if (val < (std::numeric_limits<int64_t>::min)())
        |                         ^~~~~~~~~~~~~~
  /home/linuxbrew/.linuxbrew/include/cpptoml.h:344:47: error: expected primary-expression before '>' token
    344 |         if (val < (std::numeric_limits<int64_t>::min)())
        |                                               ^
  /home/linuxbrew/.linuxbrew/include/cpptoml.h:344:50: error: '::min' has not been declared; did you mean 'std::min'?
    344 |         if (val < (std::numeric_limits<int64_t>::min)())
        |                                                  ^~~
        |                                                  std::min
  In file included from /home/linuxbrew/.linuxbrew/Cellar/gcc/11.3.0_1/include/c++/11/algorithm:62,
                   from /home/linuxbrew/.linuxbrew/include/cpptoml.h:10,
                   from /tmp/watchman-20220607-31055-19g08zv/watchman-2022.06.06.00/watchman/watcher/eden.cpp:8:
  /home/linuxbrew/.linuxbrew/Cellar/gcc/11.3.0_1/include/c++/11/bits/stl_algo.h:3455:5: note: 'std::min' declared here
   3455 |     min(initializer_list<_Tp> __l, _Compare __comp)
        |     ^~~

Let's skip this for now.

@carlocab
Copy link
Copy Markdown
Member

carlocab commented Jun 7, 2022

Not sure why that's happening, since we have

set(CMAKE_CXX_STANDARD 17)

in fbthrift. Maybe we needed to set c++17 in cpptoml too?

@cho-m
Copy link
Copy Markdown
Member Author

cho-m commented Jun 7, 2022

watchman is using a forked copy of cpptoml for their build system, which has fix for skystrife/cpptoml#126

https://github.com/facebook/watchman/blob/v2022.06.06.00/build/fbcode_builder/manifests/cpptoml#L8

url = https://github.com/chadaustin/cpptoml/archive/refs/tags/v0.1.2.tar.gz

We could just patch Homebrew's cpptoml with fix. There is an upstream PR skystrife/cpptoml#123 but looks like project may not be actively maintained.

@carlocab
Copy link
Copy Markdown
Member

carlocab commented Jun 7, 2022

We could just patch Homebrew's cpptoml with fix.

It's just an additional #include, so I guess that's fine.

@BrewTestBot
Copy link
Copy Markdown
Contributor

🤖 A scheduled task has triggered a merge.

@github-actions github-actions Bot added the outdated PR was locked due to age label Jul 8, 2022
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jul 8, 2022
@cho-m cho-m deleted the fbthrift-gcc11 branch September 18, 2022 06:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants