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

Jump to bottom

boost: Improve option for i18n backend #5553

Merged
merged 7 commits into from
May 28, 2021
Merged

boost: Improve option for i18n backend #5553

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e3d49e4
Improve boost option for i18n backend
lieser May 18, 2021
bf8b94c
ignore os.subsystem
lieser May 26, 2021
ef7676f
use "libiconv"/"off" instead of True/False
lieser May 26, 2021
ab07818
remove options if locale disabled
lieser May 26, 2021
eb56727
fail on missing backend and fixes
lieser May 26, 2021
30ec4ce
default on Macos to libiconv
lieser May 26, 2021
9e28bac
add patch for 1.76.0 too
lieser May 27, 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
62 changes: 46 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 @@ -92,7 +92,9 @@ class BoostConan(ConanFile):
"debug_level": [i for i in range(0, 14)],
"pch": [True, False],
"extra_b2_flags": "ANY", # custom b2 flags
"i18n_backend": ["iconv", "icu", None],
"i18n_backend": ["iconv", "icu", None, "deprecated"],
"i18n_backend_iconv": ["libc", True, False],
"i18n_backend_icu": [True, False],
"visibility": ["global", "protected", "hidden"],
}
options.update({"without_{}".format(_name): [True, False] for _name in CONFIGURE_OPTIONS})
Expand Down Expand Up @@ -122,7 +124,9 @@ class BoostConan(ConanFile):
"debug_level": 0,
"pch": True,
"extra_b2_flags": "None",
"i18n_backend": "iconv",
"i18n_backend": "deprecated",
"i18n_backend_iconv": "libc",
"i18n_backend_icu": False,
"visibility": "hidden",
}
default_options.update({"without_{}".format(_name): False for _name in CONFIGURE_OPTIONS})
Expand Down Expand Up @@ -225,6 +229,12 @@ def _python_executable(self):
exe = self.options.python_executable if self.options.python_executable else sys.executable
return str(exe).replace('\\', '/')

@property
def _is_windows_platform(self):
return ((self.settings.os == "Windows") and
(self.settings.os.subsystem in [None, "cygwin"]) or
self.settings.os in ["WindowsStore", "WindowsCE"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the subsystem matter?
Isn't checking for settings.os enough?

Suggested change
return ((self.settings.os == "Windows") and
(self.settings.os.subsystem in [None, "cygwin"]) or
self.settings.os in ["WindowsStore", "WindowsCE"])
return self.settings.os in ("Windows", "WindowsStore", "WindowsCE")

If something is built for Windows, isn't that enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the wsl subsystem I would say it matters. As this is really a Linux (WM), I would expect boost to not treat it as a windows system.

As for the other subsystems, I'm not that familiar with them. But the check in #1704 did treat cygwin as being non posix (_is_posix_platform check there). And it is also handled special by the boost build system:
https://github.com/boostorg/locale/blob/ccb8fbb9a1a0dbdffb1054ffa34e4aba1e425642/build/Jamfile.v2#L271-L275

Copy link
Contributor

@madebr madebr May 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know much about wsl, but aren't applications built with wsl actually Linux applications (os=Linux)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, applications build with in the wsl should be just Linux applications. Was also surprised that the wsl did show up in that list.

I just did install conan under the wsl, and for me the default config of conan was os=Linux. Maybe someone else can explain for what os=Windows; os.subsystem=wsl is actually used, and how it should be treated.

Copy link
Contributor

Choose a reason for hiding this comment

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

WSL is capable of running:

  • most of regular Linux executables (ELF files) which use Linux syscalls
  • regular Windows executable (PE files)

the first case is definitely for os=Linux (as these compiled ELF files will be run on regular Linux, as well on WSL/WSL2)
the second is probably os=Windows; os.subsystem=wsl if you want to use build or use Windows applications via WSL layer (e.g. launching bash and other tools from WSL instead of MSYS)

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 for the explanation. I just did a test with -s os.subsystem=wsl and both iconv/icu disabled, and boost did not complaint.
But I also did not really see a difference in the output (e.g. paths there still C:\Users\.., and not something like /mnt/c/...). So unsure if I really tested it correctly.

So adding wsl to the subsystems here seems form my current understanding to be the correct thing (but I'm still not 100% sure).

Copy link
Contributor

Choose a reason for hiding this comment

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

subsystem affects a couple of functions, such as:

  • unix_path
  • run_in_windows_bash

if recipe doesn't use any of them, there will be no difference in output
as I remember, boost build system doesn't rely on bash, so subsystem might be completely irrelevant for the package
(except the fact you are using the correct compiler targeting the correct subsystem, but that's out of our control right now)

Copy link
Contributor

Choose a reason for hiding this comment

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

We had done some experiments with some of our testers, my notes match SSE4's comment https://github.com/conan-io/conan-center-index/pull/5553/files#r634440165. It makes the executable compatible with both environments.

I suspect this mode of working is not supported by Boost, https://www.boost.org/doc/libs/1_76_0/more/getting_started/windows.html notes mingw is not support which (to my knowledge) is similar.

I have no objections but I share the sentiment that perhaps if best to leave out the subsystem. Since it introduces more complexity (and Boost is already huge).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the check for the subsystem


def config_options(self):
if self.settings.os == "Windows":
del self.options.fPIC
Expand Down Expand Up @@ -253,6 +263,9 @@ def config_options(self):
self.options.without_json = True
self.options.without_nowide = True

# iconv is off by default on Windows and Solaris
if self._is_windows_platform or self.settings.os == "SunOS":
self.options.i18n_backend_iconv = False

# Remove options not supported by this version of boost
for dep_name in CONFIGURE_OPTIONS:
Expand Down Expand Up @@ -304,11 +317,23 @@ def configure(self):
elif self.options.shared:
del self.options.fPIC

if self.options.i18n_backend != "deprecated":
self.output.warn("i18n_backend option is deprecated, do not use anymore.")
if self.options.i18n_backend == "iconv":
self.options.i18n_backend_iconv = True
self.options.i18n_backend_icu = False
if self.options.i18n_backend == "icu":
self.options.i18n_backend_iconv = False
self.options.i18n_backend_icu = True
if self.options.i18n_backend == "None":
self.options.i18n_backend_iconv = False
self.options.i18n_backend_icu = False
if self.options.without_locale:
self.options.i18n_backend = None
self.options.i18n_backend_iconv = False
self.options.i18n_backend_icu = False
Copy link
Contributor

Choose a reason for hiding this comment

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

When an option dos not make sense, you can remove it del self.options.i18n_backend_iconv.
You can access it by using self.options.get_safe("i18n_backend_iconv").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

options are now deleted in that case

else:
if not self.options.i18n_backend:
raise ConanInvalidConfiguration("Boost.locale library requires a i18n_backend (either 'icu' or 'iconv')")
if not self.options.i18n_backend_iconv and not self.options.i18n_backend_icu and not self._is_windows_platform:
raise ConanInvalidConfiguration("Boost.Locale library needs either iconv or ICU library to be built on non windows platforms")

if not self.options.without_python:
if not self.options.python_version:
Expand Down Expand Up @@ -418,11 +443,11 @@ def _with_zstd(self):

@property
def _with_icu(self):
return not self.options.header_only and self._with_dependency("icu") and self.options.i18n_backend == "icu"
return not self.options.header_only and self._with_dependency("icu") and self.options.i18n_backend_icu

@property
def _with_iconv(self):
return not self.options.header_only and self._with_dependency("iconv") and self.options.i18n_backend == "iconv"
return not self.options.header_only and self._with_dependency("iconv") and self.options.i18n_backend_iconv == True

def requirements(self):
if self._with_zlib:
Expand All @@ -436,10 +461,12 @@ def requirements(self):

if self._with_icu:
self.requires("icu/68.2")
elif self._with_iconv:
if self._with_iconv:
self.requires("libiconv/1.16")

def package_id(self):
del self.info.options.i18n_backend

if self.options.header_only:
self.info.header_only()
self.info.options.header_only = True
Expand Down Expand Up @@ -837,15 +864,16 @@ def _build_flags(self):
flags.append("-sNO_LZMA=%s" % ("0" if self._with_lzma else "1"))
flags.append("-sNO_ZSTD=%s" % ("0" if self._with_zstd else "1"))

if self.options.i18n_backend == 'icu':
flags.append("-sICU_PATH={}".format(self.deps_cpp_info["icu"].rootpath))
flags.append("boost.locale.iconv=off boost.locale.icu=on")
elif self.options.i18n_backend == 'iconv':
flags.append("boost.locale.iconv=on boost.locale.icu=off")
if self.options.i18n_backend_icu:
flags.append("boost.locale.icu=on")
else:
flags.append("boost.locale.icu=off")
flags.append("--disable-icu")
if self.options.i18n_backend_iconv:
flags.append("boost.locale.iconv=on")
else:
flags.append("boost.locale.iconv=off boost.locale.icu=off")
flags.append("--disable-icu --disable-iconv")
flags.append("boost.locale.iconv=off")
flags.append("--disable-iconv")

def add_defines(library):
for define in self.deps_cpp_info[library].defines:
Expand Down Expand Up @@ -1389,7 +1417,9 @@ def filter_transform_module_libraries(names):
if conan_requirement not in self.requires:
continue
if module == "locale" and requirement in ("icu", "iconv"):
if requirement != self.options.i18n_backend:
if requirement == "icu" and not self._with_icu:
continue
if requirement == "iconv" and not self._with_iconv:
continue
self.cpp_info.components[module].requires.append("{0}::{0}".format(conan_requirement))
for incomplete_component in incomplete_components:
Expand Down