-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: Improve option for i18n backend #5553
boost: Improve option for i18n backend #5553
Conversation
I detected other pull requests that are modifying boost/all recipe:
This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
recipes/boost/all/conanfile.py
Outdated
return ((self.settings.os == "Windows") and | ||
(self.settings.os.subsystem in [None, "cygwin"]) or | ||
self.settings.os in ["WindowsStore", "WindowsCE"]) |
There was a problem hiding this comment.
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?
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I am not sure if I like
another things I don't understand from the docs if backends are completely orthogonal (mutually exclusive), or is it possible to activate multiple backends at once (e.g. both ICU and iconv)? last thing, I don't like, is "soft" fallbacks, then I request one configuration with some backend, but boost silently switches to another configuration I didn't request instead of failure. that introduces binaries incorrectly labeled (e.g. package id indicates there is a dependency on libiconv, but there isn't any, as boost suddenly felt back to libc or winapi). /cc @madebr @grafikrobot for their input |
Yes, you can enable both ICU and iconv backend at the same time. Which is the reason I added separate options for it, instead of having on As for the iconv value, maybe something like And I would not add extra options for disabling
I'm totally with you on that. But like I wrote, I did not really find a good solution for that. |
Just to make sure, I tried it with both enabled on windows. In the call to But the build seems to fail to detect ICU:
I will look into it next week to see what is going wrong. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in favor of changing the defaults since it removes the LGPL from the boost recipes 😄
but boost is not LGPL, it's BSL |
libiconv requirement changes that from the enterprise consumer POV |
I now included a patch for Boostl:local to fail if the test for the requested backend fails. I also investigated the local failed build with ICU backend on windows. As this is not directly related to the issue I'm trying to solve here, and should have not worked before either, I would like to leave a fix for that out of this PR. |
This comment has been minimized.
This comment has been minimized.
Seem like on Macos the c library does not include iconv. Switched there to |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
All green in build 8 (
|
* Improve boost option for i18n backend * ignore os.subsystem * use "libiconv"/"off" instead of True/False * remove options if locale disabled * fail on missing backend and fixes * default on Macos to libiconv * add patch for 1.76.0 too
i18n_backend
option and add separate ones foriconv
/icu
posix
,std
andwinapi
backend.iconv
backend, add the possibility to use the one included in libc instead of libiconviconv
/icu
backends and make it the default ([question] Boost: default i18n_backend on Windows #5528)Below are some original notes for this PR, which are partly outdated.
As far as I can tell boost sadly:
iconv
from libc or libiconviconv
/icu
backend is set to on, and boost fails not find it, compilation will still workThe above means that:
iconv
can be set True (i.e. as external lib), but boost will still use the one from libciconv
can be set to use the one from glibc, but silently use an external one, or (i.e. on windows) without it at alliconv
/icu
can be set to True, but boost may still be build without them if it fails to find themNote that the first one was already an issue in the current recipe. I think it would be good to get an error, or at least a warning.
Unfortunately I can't think of an nice and easy solution.
iconv (libc) : yes
. Don't know if Conan already support that, and if that is something that recipes should do.Maybe this could also be address in a separate PR later on.
See also the boost documentation and build file for reference:
https://www.boost.org/doc/libs/1_76_0/libs/locale/doc/html/building_boost_locale.html
https://github.com/boostorg/locale/blob/develop/build/Jamfile.v2
conan-center hook activated.