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.

Sign up for GitHub

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
Merged

boost: add boost/1.76.0 + bump b2 #5246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bbf27cc
boost: bump b2 build dependency
madebr Apr 17, 2021
8ff7252
boost: add boost/1.76.0
madebr Apr 17, 2021
b29e63a
boost: add 1.76.0 to config.yml
madebr Apr 17, 2021
6771590
boost: move check to validate() + Boost.Math requires c++11
madebr Apr 17, 2021
8d92874
boost: lower minimum required conan version
madebr Apr 18, 2021
6a204f2
boost: remove dead comment
madebr Apr 18, 2021
2ba4241
boost: assume apple-clang will never implement c++11
madebr Apr 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions recipes/boost/all/conandata.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ sources:
"https://sourceforge.net/projects/boost/files/boost/1.75.0/boost_1_75_0.tar.bz2"
]
sha256: "953db31e016db7bb207f11432bef7df100516eeb746843fa0486a222e3fd49cb"
1.76.0:
url: [
"https://dl.bintray.com/boostorg/release/1.76.0/source/boost_1_76_0.tar.bz2",
"https://boostorg.jfrog.io/artifactory/main/release/1.76.0/source/boost_1_76_0.tar.bz2",
"https://sourceforge.net/projects/boost/files/boost/1.76.0/boost_1_76_0.tar.bz2"
]
sha256: "f0397ba6e982c4450f27bf32a2a83292aba035b827a5623a14636ea583318c41"
patches:
1.69.0:
- patch_file: "patches/boost_build_asmflags.patch"
Expand Down
78 changes: 62 additions & 16 deletions recipes/boost/all/conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.



# When adding (or removing) an option, also add this option to the list in
Expand Down Expand Up @@ -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

return 99
return {
"gcc": 6,
"clang": 6,
Expand Down Expand Up @@ -180,6 +183,19 @@ def _all_dependent_modules(self, name):
break
return dependencies

def _all_super_modules(self, name):
dependencies = {name}
while True:
new_dependencies = set(dependencies)
for module in self._dependencies["dependencies"]:
if dependencies.intersection(set(self._dependencies["dependencies"][module])):
new_dependencies.add(module)
if len(new_dependencies) > len(dependencies):
dependencies = new_dependencies
else:
break
return dependencies

@property
def _source_subfolder(self):
return "source_subfolder"
Expand Down Expand Up @@ -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

# Starting from 1.76.0, Boost.Math requires a c++11 capable compiler
# ==> disable it by default for older compilers or c++ standards

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
Comment on lines +270 to +276
Copy link
Contributor

@SpaceIm SpaceIm Apr 23, 2021 •
edited
Loading

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.


if self.settings.compiler.cppstd:
if not tools.valid_min_cppstd(self, 11):
disable_math()
else:
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()
Comment on lines +282 to +286
Copy link
Contributor

@SpaceIm SpaceIm Apr 23, 2021 •
edited
Loading

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 •
edited
Loading

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


@property
def _configure_options(self):
return self._dependencies["configure_options"]
Expand All @@ -272,6 +310,18 @@ def configure(self):
if not self.options.i18n_backend:
raise ConanInvalidConfiguration("Boost.locale library requires a i18n_backend (either 'icu' or 'iconv')")

if not self.options.without_python:
if not self.options.python_version:
self.options.python_version = self._detect_python_version()
self.options.python_executable = self._python_executable

if self.options.layout == "b2-default":
self.options.layout = "versioned" if self.settings.os == "Windows" else "system"

if self.options.without_fiber:
del self.options.numa

def validate(self):
if not self.options.multithreading:
# * For the reason 'thread' is deactivate look at https://stackoverflow.com/a/20991533
# Look also on the comments of the answer for more details
Expand All @@ -294,11 +344,6 @@ def configure(self):
if self.options.get_safe("without_{}".format(mod_dep), False):
raise ConanInvalidConfiguration("{} requires {}: {} is disabled".format(mod_name, mod_deps, mod_dep))

if not self.options.without_python:
if not self.options.python_version:
self.options.python_version = self._detect_python_version()
self.options.python_executable = self._python_executable

if not self.options.get_safe("without_nowide", True):
# nowide require a c++11-able compiler with movable std::fstream
mincompiler_version = self._min_compiler_version_nowide
Expand All @@ -317,10 +362,6 @@ def configure(self):
self.output.warn("I don't know what the default c++ standard of this compiler is. I suppose it supports c++11 by default.\n"
"This might cause some boost libraries not being built and conan components to fail.")

# FIXME: check this + shouldn't default be on self._is_msvc?
# if self.options.layout == "b2-default":
# self.options.layout = "versioned" if self.settings.os == "Windows" else "system"

if not all((self.options.without_fiber, self.options.get_safe("without_json", True))):
# fiber/json require a c++11-able compiler.
if self.settings.compiler.cppstd:
Expand All @@ -334,15 +375,20 @@ def configure(self):
self.output.warn("I don't know what the default c++ standard of this compiler is. I suppose it supports c++11 by default.\n"
"This might cause some boost libraries not being built and conan components to fail.")

if self.options.layout == "b2-default":
self.options.layout = "versioned" if self.settings.os == "Windows" else "system"

if self.options.without_fiber:
del self.options.numa
if tools.Version(self.version) >= "1.76.0":
# Starting from 1.76.0, Boost.Math requires a c++11 capable compiler
if not self.options.without_math:
if self.settings.compiler.cppstd:
tools.check_min_cppstd(self, 11)
else:
min_compiler_version = self._min_compiler_version_default_cxx11
if min_compiler_version is not None:
if tools.Version(self.settings.compiler.version) < min_compiler_version:
raise ConanInvalidConfiguration("Boost.Math requires a C++11 capable compiler")

def build_requirements(self):
if not self.options.header_only:
self.build_requires("b2/4.2.0")
self.build_requires("b2/4.5.0")

def _with_dependency(self, dependency):
"""
Expand Down
Loading