-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix for forced libiconv in non Visual Studio environments #1
base: testing/3.2.1
Are you sure you want to change the base?
Conversation
The problem with forcing libiconv (LGPL 2.1) is mainly a matter of licensing, making impossible to statically build gdal (MIT license) in enviroments where LGPL is not permitted. The proposed fix honors the option to disable libiconv.
By the way, not sure is ok to do the PR here or directly to conan-center, but as it seems you are the main maintainer I decided to do it here. Let me know if this is not the correct place! Thanks. |
Hi, thanks for the PR. You're right, libiconv can be disabled in autoconf build You can submit your modifications in conan-center-index, or I can open a PR, as you wish. And I would also replace: if self.options.get_safe("with_libiconv", True):
self.requires("libiconv/1.16") by if self.options.with_libiconv:
self.requires("libiconv/1.16") |
Hi @SpaceIm, Thanks! ok I can do it, probably will tag you to review there too. Are you planning to open a PR for gdal 3.3.1 in the near term? |
Yes, but due to the amount of patches required for each version, it takes time. I've also seen that, since 3.3.0, several drivers were moved to https://github.com/OSGeo/gdal-extra-drivers, so it requires extra work. |
if self.options.with_libiconv: | ||
args.append("--with-libiconv-prefix={}".format(tools.unix_path(self.deps_cpp_info["libiconv"].rootpath))) | ||
else: | ||
args.append("--without-libiconv-prefix") |
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.
if self.options.with_libiconv: | |
args.append("--with-libiconv-prefix={}".format(tools.unix_path(self.deps_cpp_info["libiconv"].rootpath))) | |
else: | |
args.append("--without-libiconv-prefix") | |
args.append("--with-libiconv-prefix={}".format(tools.unix_path(self.deps_cpp_info["libiconv"].rootpath) if self.options.with_libiconv else "no")) |
can be simplified in one line
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 think you actually need to be verbose in disabling it. Meaning explicitely call "without-libiconv-prefix".
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.
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.
--with-libiconv-prefix=no
should be the same than --without-libiconv-prefix
Hi @SpaceIm
First of all thanks for your support making gdal available through conan. We are experiencing some licensing issues due to the fact that the conan recipe is not honoring the
--with-libiconv
option. I'm not sure what is the reason for disabling this option in non VisualStudio builds. This PR addresses the problem we have, but wondering why it was done in the first place.Thanks a lot.
Commit message:
The problem with forcing libiconv (LGPL 2.1) is mainly a matter of licensing, making impossible to statically build gdal (MIT license) in enviroments where LGPL is not permitted. The proposed fix honors the option to disable libiconv.