pytries / datrie

Fast, efficiently stored Trie for Python. Uses libdatrie.
http://pypi.python.org/pypi/datrie/
GNU Lesser General Public License v2.1
530 stars 88 forks source link

cmake build and cross-platform github CI (test, wheels, conda) #91

Open sarnold opened 3 years ago

sarnold commented 3 years ago

This actually takes care of several issues/PRs (some open, some closed).

Changes:

KOLANICH commented 3 years ago

Why should we introduce a yet another pretty large dependency, if as it is it is already builds pretty fine? CMake is usually needed for projects that rely on CMake modules (usually in its stdlib) heavily. But libdatrie is not built with CMake and doesn't use CMake routines to generate a CMake module for its autodiscovery. I see no point in using CMake here.

KOLANICH commented 3 years ago

BTW, FetchContent is a kind of insecure thing, because it automatically runs CMakeLists.txt in the content it fetches and I found no way to disable it.

sarnold commented 3 years ago

That was more-or-less the most workable thing I could come up with, BUT, it's already gone now (the submodule is back). In this case, the main reason for the whole cmake thing is having a consistent and portable build across multiple platforms and toolchains. Sadly the state of libdatrie OS packages (either lacking or broken) makes it somewhat less of a "win", but it does the right things in the ci examples:

sarnold commented 3 years ago

Well, between your feedback and some conda-forge discussion I changed my mind about the git submodule thing. I'm going to put that back and adjust the cmake bits.

sarnold commented 3 years ago

Generated a random (flaky CI) error with hypothesis on macos:

  =================================== FAILURES ===================================
  _____________________________ test_pickle_unpickle _____________________________

      @given(printable_strings)
  >   def test_pickle_unpickle(words):

  tests/test_random.py:34: 
  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
  .tox/check/lib/python3.9/site-packages/hypothesis/core.py:658: in execute_once
      self.__flaky(
  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

  self = <hypothesis.core.StateForActualGivenExecution object at 0x106b87130>
  message = 'Hypothesis test_pickle_unpickle(words=[\'\', \'Xo?y)y\\r<8PDU.eV6Hn)`VJ\', \'0\', \'"P1WTL-\']) produces unreliable results: Falsified on the first call but did not on a subsequent one'

      def __flaky(self, message):
          if len(self.falsifying_examples) <= 1:
  >           raise Flaky(message)
  E           hypothesis.errors.Flaky: Hypothesis test_pickle_unpickle(words=['', 'Xo?y)y\r<8PDU.eV6Hn)`VJ', '0', '"P1WTL-']) produces unreliable results: Falsified on the first call but did not on a subsequent one

  .tox/check/lib/python3.9/site-packages/hypothesis/core.py:861: Flaky
  ---------------------------------- Hypothesis ----------------------------------
  Falsifying example: test_pickle_unpickle(
      words=['', 'Xo?y)y\r<8PDU.eV6Hn)`VJ', '0', '"P1WTL-'],
  )
  Unreliable test timings! On an initial run, this test took 326.87ms, which exceeded the deadline of 200.00ms, but on a subsequent run it took 2.44 ms, which did not. If you expect this sort of variability in your test timings, consider turning deadlines off for this test by setting deadline=None.

  You can reproduce this example by temporarily adding @reproduce_failure('6.2.0', b'AXicFcorD4NAEADhnb29PQRJDaJpEAheggpcE3RNbatx9SSYU/3tBTP5xCAwOh8uxqrMZxanNApjb4zK6JT8nSIvp4945Joib7ZZeSp9IN+SSiAgkgznYTTKgSHQOr+9du5yPH/uoAiu') as a decorator on your test case
  =========================== short test summary info ============================
  FAILED tests/test_random.py::test_pickle_unpickle - hypothesis.errors.Flaky: ...
  ======================== 1 failed, 37 passed in 48.37s =========================
  ERROR: InvocationError for command /Users/runner/work/datrie/datrie/.tox/check/bin/pytest -v (exited with code 1)
___________________________________ summary ____________________________________
ERROR:   check: commands failed
sarnold commented 3 years ago

Why should we introduce a yet another pretty large dependency, if as it is it is already builds pretty fine? CMake is usually needed for projects that rely on CMake modules (usually in its stdlib) heavily. But libdatrie is not built with CMake and doesn't use CMake routines to generate a CMake module for its autodiscovery. I see no point in using CMake here.

You're absolutely right about upstream not using cmake BUT cmake is used by thirdparty packagers (eg, vcpkg and brew) and in this case we have to use cmake pkg-config or a custom findDatrie module. The bigger "win" is what I mentioned previously (ie, clean and consumable cross-platform builds/pkgs).

I really don't mean to seem like I'm pushing this on you, I just found it very beneficial for a couple of projects (with big dependency chains and cross-platform reqs) so I'm just trying to share it back upstream.