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

[libiconv] conan v2 update #13263

Merged
merged 7 commits into from
Oct 5, 2022

Conversation

planetmarshall
Copy link
Contributor

Specify library name and version: libiconv

Conan v2 and toolchain updates


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the conan-center hook activated.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@planetmarshall planetmarshall mentioned this pull request Oct 2, 2022
4 tasks
Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

looks super good, jsut one small nit

@prince-chrismc
Copy link
Contributor

Pretty sure this counts towards #12888

@conan-center-bot

This comment has been minimized.

env.define("NM", "dumpbin -symbols")
env.define("win32_target", "_WIN32_WINNT_VISTA")

if is_msvc(self) and Version(self.settings.compiler.version) >= "12":
Copy link
Contributor

@jwillikers jwillikers Oct 3, 2022

Choose a reason for hiding this comment

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

This isn't going to work correctly for the msvc compiler, only Visual Studio. Unfortunately, there won't be a more helpful solution until Conan 1.53.0 for this: conan-io/conan#11158

Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

recipes/libiconv/all/test_v1_package/CMakeLists.txt Outdated Show resolved Hide resolved
Co-authored-by: Jordan Williams <jordan@jwillikers.com>
@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

All green in build 6 (7b97959cec66647ed61cfdb34cc2e0f7b2479216):

  • libiconv/1.15@:
    All packages built successfully! (All logs)

  • libiconv/1.16@:
    All packages built successfully! (All logs)

  • libiconv/1.17@:
    All packages built successfully! (All logs)

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot conan-center-bot merged commit 9499396 into conan-io:master Oct 5, 2022
@planetmarshall planetmarshall deleted the iconv-modernize branch October 5, 2022 11:15
Comment on lines +100 to +101
env = VirtualBuildEnv(self)
env.generate()
Copy link
Contributor

@SpaceIm SpaceIm Oct 6, 2022

Choose a reason for hiding this comment

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

should be called before AutotoolsToolchain, otherwise it can defeat msvc env var tricks above.

Copy link
Member

Choose a reason for hiding this comment

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

Good to know! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@SpaceIm @uilianries this is still called after AutotoolsToolchain,
can this recipe be fixed up to be done the "proper" way?
I am trying to use this recipe as a model for upgrading libsodium, and its not good if this recipe isn't "gold standard" (or suggests another to follow)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, how does this even work properly? "env" variable name is reused...
first as env = tc.environment() and then as env=VirtualBuildEnv(self).
how should it be done "before AutotoolsToolchain" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Use the paclage templates as a refernce point, https://github.com/conan-io/conan-center-index/blob/master/docs/package_templates/autotools_package/all/conanfile.py#L91

we are trying to focus on keeping those up to spec


def source(self):
tools.get(**self.conan_data["sources"][self.version], destination=self._source_subfolder, strip_root=True)
def generate(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to move this after source(), otherwise recipe execution flow is a little bit confusing

System-Arch pushed a commit to System-Arch/conan-center-index that referenced this pull request Oct 7, 2022
* [libiconv] update to v2 toolchain

* [libiconv] update test packages vor v2

* [libiconv] fix cross building check

* [libiconv] supply '-FS' flag for msvc debug

* [libiconv] add destination to source method

* Update recipes/libiconv/all/test_v1_package/CMakeLists.txt

Co-authored-by: Jordan Williams <jordan@jwillikers.com>

* [libiconv] handle msvc compiler version

Co-authored-by: Jordan Williams <jordan@jwillikers.com>
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.

7 participants