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

Add farmhash #7328

Merged
merged 5 commits into from
Sep 28, 2021
Merged

Add farmhash #7328

merged 5 commits into from
Sep 28, 2021

Conversation

ericriff
Copy link
Contributor

Specify library name and version: farmhash

One more step towards tensorflow-lite #7165


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

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@ericriff
Copy link
Contributor Author

Help from a Windows developer is welcome here.

@conan-center-bot
Copy link
Collaborator

All green in build 7 (934384ceb2020fc4676ccd82348bf6689e15e7b8):

  • farmhash/cci.20190513@:
    All packages built successfully! (All logs)

@ericriff
Copy link
Contributor Author

I no longer need this package and I don't know what is going on with Windows (I don't have a windows PC to test this). Is it OK if we merge this recipe without windows support?
I can't spend much time on it and as I said I no longer need it.

@SSE4
Copy link
Contributor

SSE4 commented Sep 27, 2021

I no longer need this package and I don't know what is going on with Windows (I don't have a windows PC to test this). Is it OK if we merge this recipe without windows support?
I can't spend much time on it and as I said I no longer need it.

in general, this is perfectly find, if very first iteration doesn't support all platforms.
someone who actually needs Windows may add support in a follow up PRs.

for the farmhash in particular, I've tried to build it for Windows/VS, but it doesn't build out of the box.
for whose would like to continue the effort, the initial patch is:

diff --git a/recipes/farmhash/all/conanfile.py b/recipes/farmhash/all/conanfile.py
index 859923b3b..a65885dbc 100644
--- a/recipes/farmhash/all/conanfile.py
+++ b/recipes/farmhash/all/conanfile.py
@@ -1,4 +1,5 @@
 from conans import ConanFile, tools, AutoToolsBuildEnvironment
+import contextlib
 import os
 
 required_conan_version = ">=1.33.0"
@@ -44,11 +45,31 @@ class farmhashConan(ConanFile):
     def build_requirements(self):
         if self._settings_build.os == "Windows" and not tools.get_env("CONAN_BASH_PATH"):
             self.build_requires("msys2/cci.latest")
+        if self.settings.compiler == "Visual Studio":
+            self.build_requires("automake/1.16.3")
 
     def source(self):
         tools.get(**self.conan_data["sources"][self.version], strip_root=True,
                   destination=self._source_subfolder)
 
+    @property
+    def _user_info_build(self):
+        return getattr(self, "user_info_build", self.deps_user_info)
+
+    @contextlib.contextmanager
+    def _build_context(self):
+        if self.settings.compiler == "Visual Studio":
+            with tools.vcvars(self):
+                env = {
+                    "CC": "{} cl -nologo".format(tools.unix_path(self._user_info_build["automake"].compile)),
+                    "CXX": "{} cl -nologo".format(tools.unix_path(self._user_info_build["automake"].compile)),
+                    "CPP": "{} cl -E".format(tools.unix_path(self._user_info_build["automake"].compile)),
+                }
+                with tools.environment_append(env):
+                    yield
+        else:
+            yield
+
     def _configure_autotools(self):
         if self._autotools:
             return self._autotools
@@ -58,18 +79,21 @@ class farmhashConan(ConanFile):
             conf_args.extend(["--enable-shared", "--disable-static"])
         else:
             conf_args.extend(["--disable-shared", "--enable-static"])
-        if self.options.no_builtin_expect:
+        if self.options.no_builtin_expect or self.settings.compiler == "Visual Studio":
             self._autotools.defines.append("FARMHASH_NO_BUILTIN_EXPECT")
+            self._autotools.defines.append("__builtin_unreachable=abort")
         self._autotools.configure(configure_dir=self._source_subfolder, args=conf_args)
         return self._autotools
 
     def build(self):
-        autotools = self._configure_autotools()
-        autotools.make()
+        with self._build_context():
+            autotools = self._configure_autotools()
+            autotools.make()
 
     def package(self):
-        autotools = self._configure_autotools()
-        autotools.install()
+        with self._build_context():
+            autotools = self._configure_autotools()
+            autotools.install()
         self.copy("COPYING", src=self._source_subfolder, dst="licenses")
         tools.remove_files_by_mask(os.path.join(self.package_folder, "lib"), "*.la")
         tools.rmdir(os.path.join(self.package_folder, "share"))

it will at least start to build for VS. but there will be many compiler errors of the same kind:

C:\users\sse4\.conan\data\farmhash\cci.20190513\_\_\build\3fb49604f9c2f729b85ba3115852006824e72cab\source_subfolder\src\farmhash.cc(2012): error C2872: 'data': ambiguous symbol
C:\users\sse4\.conan\data\farmhash\cci.20190513\_\_\build\3fb49604f9c2f729b85ba3115852006824e72cab\source_subfolder\src\farmhash.cc(1996): note: could be 'char data[1048576]'
C:\users\sse4\.conan\data\farmhash\cci.20190513\_\_\build\3fb49604f9c2f729b85ba3115852006824e72cab\source_subfolder\src\farmhash.cc(2012): note: or       'data'

it seems like upstream just doesn't support Visual Studio out of the box, and it may require some patching.

@SSE4 SSE4 requested a review from uilianries September 27, 2021 05:28
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.

Yes, any future improvement will be welcome too. Thanks for providing it.

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.

There's a typo but not worth fixing... next PR hopefully


required_conan_version = ">=1.33.0"

class farmhashConan(ConanFile):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class farmhashConan(ConanFile):
class FarmhashConan(ConanFile):

@conan-center-bot conan-center-bot merged commit a3c2b3f into conan-io:master Sep 28, 2021
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.

5 participants