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

Fix for forced libiconv in non Visual Studio environments #1

Open
wants to merge 1 commit into
base: testing/3.2.1
Choose a base branch
from

Conversation

jtorresfabra
Copy link

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.

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.
@jtorresfabra
Copy link
Author

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.

@SpaceIm
Copy link
Owner

SpaceIm commented Jul 29, 2021

Hi, thanks for the PR. You're right, libiconv can be disabled in autoconf build
I can merge here, but the reference is conan-center-index. I'm using this repo to experiment few things before submitting modifications of gdal recipe to conan-center-index, and actually this repo lacks few recent improvements of CCI recipe.

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")

@jtorresfabra
Copy link
Author

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?

@SpaceIm
Copy link
Owner

SpaceIm commented Jul 29, 2021

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.

Comment on lines +618 to +621
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")
Copy link
Owner

@SpaceIm SpaceIm Jul 29, 2021

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Author

@jtorresfabra jtorresfabra Jul 29, 2021

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".

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Owner

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

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.

2 participants