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

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

sarnold
Copy link

@sarnold sarnold commented Feb 16, 2021

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

Changes:

  • pep517 with cmake extension builder (py36 - py39)
  • default is build from local git submodule (clones pybind11 if not installed, mainly for ci speedup)
  • set HAVE_LIBDATRIE_PKG=anything to use OS libdatrie instead
  • github CI workflows for test, wheels, release (linux macos windows)
  • plus a conda recipe and workflow

* decouple from submodule and use system libdatrie/dev-package instead
* chg: add test from installed wheel (ci only on ubuntu for now)
#2)

* add cmake module to find libdatrie, remove git submodule
* remove vcpkg install from macos, only use lib pkg on linux
* add more workflows for manylinux wheels and release
@KOLANICH
Copy link

KOLANICH commented Feb 16, 2021

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
Copy link

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
Copy link
Author

sarnold commented Feb 16, 2021

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:

  • on Linux it builds with gcc or clang
  • on macos it builds with xcode/clang
  • on windows it builds with msvc
  • on X platform with a valid package for libdatrie you can set HAVE_LIBDATRIE_PKG in the env to disable the submodule bits and it will use FindDatrie to find the OS pkg

@sarnold
Copy link
Author

sarnold commented Feb 17, 2021

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
Copy link
Author

sarnold commented Feb 17, 2021

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

* restore git submodule, set commit d1dfdb8, make sure path uses https url
* switch cmake bits to use submodule hdrs/srcs, let cmake init the submodule if empty
* add updated conda recipe and corresponding ci workflow
Signed-off-by: Stephen L Arnold <[email protected]>
@sarnold sarnold changed the title cmake build with FetchContent fallback cmake build and cross-platform github CI (test, wheels, conda) Feb 17, 2021
@sarnold
Copy link
Author

sarnold commented Feb 18, 2021

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.

* new: add updated conda recipe and corresponding ci workflow
* chg: restore git submodule, set commit d1dfdb8 (uses github relative path)
* new: dev: add support for generating test coverage data
* update cmake build flags for cython if WITH_COVERAGE
* ci uses platform host pybind11, libdatrie (linux-only)
* update cmake option handling, set version info
* add cmake cmd to copy inplace extension to src/ (coverage only)
* fix: dev: add test decorator for macos taking longer on a test
* fix: dev: add missing vcpkg action param (not in the readme)
* setupOnly *requires* vcpkgGitCommitId (only in hosted examples)
* use environment.devenv.yml with condadev
* fix: dev: "flaky" test failed again, extend deadline to 2500 ms
* macos has occasional lag issues with disk I/O ?
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