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

boost: add boost/1.76.0 + bump b2 #5246

Merged
merged 7 commits into from
Apr 27, 2021

Conversation

madebr
Copy link
Contributor

@madebr madebr commented Apr 17, 2021

Specify library name and version: boost/1.76.0

Fixes #5272

Release notes: https://www.boost.org/users/history/version_1_76_0.html

No new libraries!


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@conan-center-bot

This comment has been minimized.

@ghost
Copy link

ghost commented Apr 17, 2021

@conan-center-bot

This comment has been minimized.

@madebr
Copy link
Contributor Author

madebr commented Apr 17, 2021

@jgsogo @uilianries
Boost/1.71.0 failed in test_package + log contains

MSBUILD : error MSB4166: Child node "2" exited prematurely. Shutting down. Diagnostic information may be found in files in the temporary files directory named MSBuild_*.failure.txt.

So I believe this is a CI infrastructure problem.

@madebr madebr closed this Apr 17, 2021
@madebr madebr reopened this Apr 17, 2021
@madebr madebr reopened this Apr 17, 2021
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@prince-chrismc
Copy link
Contributor

prince-chrismc
prince-chrismc previously approved these changes Apr 18, 2021
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@madebr
Copy link
Contributor Author

madebr commented Apr 19, 2021

1.74.0 failure remains a mystery to me 😄

@prince-chrismc
Copy link
Contributor

Odd it passed before.... might be a bug that's been hiding!

Last last retry this time I feel it!

@madebr
Copy link
Contributor Author

madebr commented Apr 19, 2021

“Insanity is doing the same thing, over and over again, but expecting different results.”

@madebr madebr closed this Apr 19, 2021
@madebr madebr reopened this Apr 19, 2021
@conan-center-bot

This comment has been minimized.

@madebr
Copy link
Contributor Author

madebr commented Apr 19, 2021

Ok, it's official.
I'm admitting myself to a psychic center.

@@ -139,6 +139,9 @@ def export(self):
@property
def _min_compiler_version_default_cxx11(self):
# Minimum compiler version having c++ standard >= 11
if self.settings.compiler == "apple-clang":
# For now, assume apple-clang will enable c++11 in the distant future
Copy link
Contributor

Choose a reason for hiding this comment

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

I think 12 is 14 by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant to say "c++11 by default"

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep! It's our usual problem... AFAIK, in apple-clang 12 they finally set it to c++14 by default (they skipped 11)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the (failed) build on apple-clang 12, it does not have (at least) c++11 support by default.
See https://www.phoronix.com/scan.php?page=news_item&px=EXT4-Casefolding-With-Encrypt

@jgsogo
Copy link
Contributor

jgsogo commented Apr 20, 2021

I think the Windows Build machines are working under a heavy load in a too constrained environment.

@jgsogo @danimtb
Perhaps c3i needs some logic to detect these problems and let it try to build again later (when the load has diminished).
Maybe when MSB4166 and/or MSB6006 are detected in the logs?

Maybe it is just a matter of the number of jobs we are running in parallel inside each machine. We might be hitting some memory limits when running these big builds in parallel. 🤔

I mean, instead of parsing the logs and try to understand the errors (something I would love to do), it easier to run less jobs in parallel in each windows machine. From the user perspective, it can be much better even if it takes a little bit more time, because the alternative is a failure plus close/open pull-request.

@prince-chrismc
Copy link
Contributor

We might be hitting some memory limits when running these big builds in parallel.

This particular error has been on the rise, even for medium size projects.

I think reducing the degree of parallelism is acceptable, most of the time I switch tasks while waiting for results 🙈

@madebr
Copy link
Contributor Author

madebr commented Apr 20, 2021

Maybe noteworthy is that it isn't the boost build that was failing in the last builds, but the test package.
Boost's heavy use of header files makes compiling it very memory hungry.

Perhaps lowering the parallel level of building the tests will avoid the unexpected process kills?

@conan-center-bot
Copy link
Collaborator

All green in build 12 (2ba4241e4b00452fdb2f149df599fa8e80fa6d9b):

  • boost/1.72.0@:
    All packages built successfully! (All logs)

  • boost/1.74.0@:
    All packages built successfully! (All logs)

  • boost/1.73.0@:
    All packages built successfully! (All logs)

  • boost/1.69.0@:
    All packages built successfully! (All logs)

  • boost/1.70.0@:
    All packages built successfully! (All logs)

  • boost/1.71.0@:
    All packages built successfully! (All logs)

  • boost/1.75.0@:
    All packages built successfully! (All logs)

  • boost/1.76.0@:
    All packages built successfully! (All logs)

@SSE4 SSE4 requested a review from uilianries April 21, 2021 06:07
@@ -247,6 +263,28 @@ def config_options(self):
# Shared builds of numa do not link on Visual Studio due to missing symbols
self.options.numa = False

if tools.Version(self.version) >= "1.76.0":
Copy link
Contributor

Choose a reason for hiding this comment

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

probably, in future we should add C++ standard requirements to the yml (existing or new), as different libraries tend to change their requirements across boost releases

@madebr
Copy link
Contributor Author

madebr commented Apr 23, 2021

@prince-chrismc @SpaceIm
@uilianries

Can I get some reviews please?

Comment on lines +282 to +286
min_compiler_version = self._min_compiler_version_default_cxx11
if min_compiler_version is None:
self.output.warn("Assuming the compiler supports c++11 by default")
elif tools.Version(self.settings.compiler.version) < min_compiler_version:
disable_math()
Copy link
Contributor

@SpaceIm SpaceIm Apr 23, 2021

Choose a reason for hiding this comment

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

Why not inject C++11 flag instead if math is enabled? I understand that math will be disabled if compiler default standard is less than C++11, even if it supports C++11, unless compiler.cppstd is explicitly set.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's right thing to do. if compiler.cppstd is set explicitly, it should be passed according to the setting. if it's not defined at all, it means just follow the compiler's default, whatever it is. in that case, injecting standard different from the default will only cause confusion, or even problems (as boost changes ABI for C++11 which might be undesired).

Copy link
Contributor

Choose a reason for hiding this comment

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

this is what we do indirectly in almost all libs based on CMake, since usually they force CXX_STANDARD to the minimum standard required to properly build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope that this won't cause an influx of issues reporting missing boost math libraries.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, we're doing it now, but it doesn't mean we're doing the right thing :)
I think the plan is to eventually add cppstd support for CCI and process it accordingly, instead of silently injecting -std=c++xx in some recipes.

Copy link
Contributor

@SpaceIm SpaceIm Apr 23, 2021

Choose a reason for hiding this comment

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

We don't inject this flag ourself, it's already there usually in underlying build files of libraries, this will require too much patches. I think that the pressure to recipes maintainers is highly underestimated for this feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

for boost, the flag is controlled by b2 cxxstd=<version> and it's not hard-coded. so for us, it should be a simple mapping compiler.cppstd <-> cxxstd.
for other builds systems, it will be another option (e.g. CMAKE_CXX_STANDARD in CMake or cpp_std in meson), thus similar mapping will happen in the recipe or build helper.
some projects tend to hard code specific C++ standard in their makefiles (e.g. hardcoding CXXFLAGS=-std=c++<version> or set(CMAKE_CXX_STANDARD <version>). should we do something about it?
there are two ways:

  • provide patches to remove hardcoded C++ standard flags
  • assume the hard-coded C++ standard is the only one supported by the library, thus, throw ConanInvalidConfigurations for all other compiler.cppstd values besides hard-coded one

I personally think the second approach is the most correct one.
that we're doing right now is a complete mess, and the only excuse is that CCI doesn't currently support compiler.cppstd, and if we try to do things correctly, we will be unable to build many libraries.
but so far it works fine, and we had almost no reports of it caused any actual problems.
doing things correctly, on other hand, will increase pressure on maintainers, as you pointed out, as well it will make CI more complicated and will require way more resources (and will make builds even slower, multiply boost build time by the number of active C++ standards).
probably, the benefit doesn't worth the effort right now, and there are other areas which will bring more benefit for much less effort. still, in future, some C++ standard may introduce a huge ABI break, which will make "correct" handling of C++ standard in CCI a vital and must have feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally think the second approach is the most correct one.

This is the policy we have, before CMake was the main build script it was a wild west and it's the on sensible way...

As a consumer if you build locally you expect the project to be configured as intended. It's not our place to change that value... nor test all possible combinations and patch in between cppstd

@@ -16,7 +16,7 @@
except ImportError:
from io import StringIO

required_conan_version = ">=1.28.0"
required_conan_version = ">=1.33.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
required_conan_version = ">=1.33.0"
required_conan_version = ">=1.32.0"

even lower ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

CCI policy is to require latest conan version, so 1.33 is okay, no need to change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I prefer to avoid triggering a rebuild for this trivial change 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not very important. But I disagree, required_conan_version is a useful information. It avoids cryptic errors for consumers. Moreover, think to beginners, conan is already complex, so let be honest, few people read the all documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's requiring conan 1.33.0 now. Isn't that fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

But sure, if this is the only modification, let's skip it since it takes 4 of 5 hours to build (which is not too long actually for so many versions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The biggest problem is that the build infrastructure is a bit unstable when building boost. I needed to trigger a few rebuilds before it could complete.

Comment on lines +270 to +276
def disable_math():
super_modules = self._all_super_modules("math")
for smod in super_modules:
try:
setattr(self.options, "without_{}".format(smod), True)
except ConanException:
pass
Copy link
Contributor

@SpaceIm SpaceIm Apr 23, 2021

Choose a reason for hiding this comment

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

just to be sure to understand:

  • super modules is the complete list of modules which might have math in their own dependency graph?
  • what could fail in setattr(self.options, "without_{}".format(smod), True)? Is it not under control?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just to be sure to understand:

* super modules is the complete list of modules which might have math in their own dependency graph?

Yes, the current implementation is quick and dirty and I don't guarantee it working under all combinations though.

* what could fail in `setattr(self.options, "without_{}".format(smod), True)`? Is it not under control?

The dependencies section of the dependencies-x.yy.yml file contains modules that are not valid options of boost.build (configure_options section). So I just ignore these.

@conan-center-bot conan-center-bot merged commit a5600b9 into conan-io:master Apr 27, 2021
@madebr madebr deleted the boost_1_76_0 branch April 27, 2021 10:58
AlvaroFS pushed a commit to AlvaroFS/conan-center-index that referenced this pull request May 7, 2021
* boost: bump b2 build dependency

* boost: add boost/1.76.0

* boost: add 1.76.0 to config.yml

* boost: move check to validate() + Boost.Math requires c++11

* boost: lower minimum required conan version

* boost: remove dead comment

* boost: assume apple-clang will never implement c++11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[request] boost/1.76.0
7 participants