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

Preparation for interface emitter vol. 1 #287

Merged
merged 19 commits into from
Jun 5, 2023
Merged

Conversation

safl
Copy link
Member

@safl safl commented May 10, 2023

Motivation

This PR is a collection of fixes and refactoring to prepare for:

  • Finalize Python offerings #219
  • NVMe-spec. header directly from spec to code via spex and yace
  • Command-preppers via spex + yace
  • Solid API bindings for Python and Rust via yace

These preparation refactor the API i a manner better suited for automatically generating (no headers include any other headers).

Integrate Python tests into GitHUB CI build-and-test

The pytest utilizing the ctypes/cython bindings were not integrated with the test-infrastructure, that is, the cijoe-configuration was not accessible and the xnvme_parametrize() producing backend configurations and yielding device configurations were not utilized. They are now, with a couple of exceptions and the follow caveat.

Caveat: does not work on FreeBSD. Due to the following.

  • cijoe executes command without a pseudo-terminal, on FreeBSD this causes pipx ensurepath to hang. This could be changed in cijoe.transport shell_execute(..., get_pty=True), however, this requires verification that it does not break/regress current usage.
  • Installing the Python packages requires bcrypt / cryptography, possibly Rust.

These thing are doable, but beyond the scope of this PR. The PR enables the plumbing for the Python tests to run along with the "regular" tests. That is the goal. Complete platform coverage is on todo, including improving what is described above.

@safl safl force-pushed the interface_emitter branch 11 times, most recently from 58b21b7 to 5b7ed96 Compare May 17, 2023 11:58
@safl safl force-pushed the interface_emitter branch 2 times, most recently from c53a7e6 to a01901b Compare May 22, 2023 12:30
@safl safl mentioned this pull request May 25, 2023
@safl safl added this to the v0.7.0 milestone May 26, 2023
@safl safl added the pr-ready-for-test PR is ready for functional tests label May 26, 2023
@safl safl force-pushed the interface_emitter branch 2 times, most recently from 1e173a3 to d512fd2 Compare May 27, 2023 19:46
@safl safl changed the title Interface emitter Preparation for interface emitter May 30, 2023
@safl safl force-pushed the interface_emitter branch 5 times, most recently from a4ea6ec to f2f0acd Compare June 1, 2023 13:46
@safl safl marked this pull request as ready for review June 1, 2023 13:55
@safl safl force-pushed the interface_emitter branch 4 times, most recently from 1cb229d to 3883fc6 Compare June 2, 2023 10:36
safl added 4 commits June 2, 2023 15:48
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
@safl safl force-pushed the interface_emitter branch 4 times, most recently from 3111291 to cf823fa Compare June 3, 2023 08:21
@safl safl changed the title Preparation for interface emitter Preparation for interface emitter vol. 1 Jun 3, 2023
safl added 10 commits June 5, 2023 12:56
New functionality was added to cijoe to support executing pytest
remotely. This is needed for the refactoring of the Python bindings.
Thus this change in preparation.

Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
* docs, updated with a brief description on "remote" development

* scripts/git_push_checkout.py, this script takes care of syncing local
repository with remote via git

* scripts/xnvme_clean.py, in a dev-workflow, then the same task is executed
repeatedly, thus one cannot assume that the repository is clean. Rather, one
would expect that it is not. Thus, this script to clean it.

* workflows/dev-sync-and-build.yaml, description and steps in a remote dev.
environment

* git-ignore: add 'configs/dev-*.toml' to ignore, as this will be used a
config-file derived by one of the existing configs, and the adjust to suit the
developers remote hardware

Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Recent version of cijoe-cli has '-l' and '-ll' to increase log-level,
where it previously was '-l', '-ll', and '-lll'.

Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
The instructions on building the Linux kernel was missing the
prerequisite of installing build utilities. This fixes it.

Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
With the exception of the library entry-point (libxnvme.h), then no
other API headers include other headers. That is, all of the
(libxnvme_*.h) do not include any other headers.

It is just the job of libxnvme.h to provide illustrate how to use the
library headers. E.g. which third-party / system-headers are needed by
xnvme. As a consumer of xNVMe, one now have the option of:

A) Default library consumption

  - do: "include <libxnvme.h>"
  - Include **all** of xNVMe definitions
  - Use the third-party definitions as defined by the headers included
    in "libxnvme.h"

B) Tailored library consumption

  - do: "include <libxnvme_COMPONENT.h>"
  - Prefixed with the definitions that you see fit

With the above, then the consumer of the xNVMe library can override
definitions of everything, down to the definitions of errno, uint32_t,
size_t, etc.

* adm: rm unnecessary include
* api/core,dev,opts: redo for modularization
* api/ver: rm headers
* be/internal: rm unnecessary include
* be: rm unnescesary include libxnvme.h
* buf: mv defs. / decl. into libxnvme_buf.h
* buf: rm unnescesary include libxnvme.h
* cmd: mv into libxnvme_cmd.h
* geo: mv include from hdr to src
* geo: rm unnescesary include libxnvme.h
* ident: rm unnescesary include libxnvme.h
* lba: rm unnescesary include libxnvme.h
* opts: mv desf. / decl. into libxnvme_opts.h
* spec: redo include-convention
* spec: rm unnescesary include libxnvme.h
* spec_fs: rm unnescesary include libxnvme.h
* spec_pp: rm unnescesary include libxnvme.h

This change is done in preparation for generating the xNVMe API using
YACE as well as generating ABI-compatible bindings for Python and Rust.

Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
This removes the xNVMe Cython bindings and header. The intent is to
provide a single set of Python bindings, and that these are based on a
minimal toolchain requirements, that is, the built-in ctypes FFI.  This
is not just to an un-motivated quest for minimal dependencies.  Rather,
it is to exploit the Python language for what it excels at
rapid-developement and rich interactive environments.

By using ctypes, then unlike Cython, CFFI etc., then no new
library/modules needs to be built, deployed and installed. Instead, the
Python bindings are provided readily available using the xNVMe library
as-is. Thus, far reducing the complexity of providing a
system-compatible loadable library to work with the given Python FFI.

To avoid losing history, change-tracking, then the tests implemented
using the cython-based Python bindings, are not moved + modified. Just
moved here, and in the following commit, ported to use the ctypes-based
Python bindings.

The "xnvme-core" is moved into "bindings".

* add clang2py include-path for macos

docs(py): adjust according to changes in xNVMe Python offerings

* Remove Cython header and binding descriptions
* Add a FAQ section, describing common issues with ctypes usage

refactor(cijoe): remove cython from gha_provision.py

Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
The rest of xNVMe uses the conftest provided in cijoe/tests/conftest.py,
it takes care of producing device and backend configurations for tests
based on what is available in a cijoe configuration file.

This refactoring makes use of this parametrization logic instead of
redoing it. Additionally, then the tests themselves are implemented
using xNVMe Python bindings based on Cython, this is refactored to use
Ctypes instead.

With this port then there is no fixture doing device setup, the
autofreed buffer is not used either. Replacing autofreed buffer is
"buf_and_view()".

refactor(py/tests/backends): removed

The tests within are relocated to test_enum and test_linux_hugepage.

refactor(py/tests/basics): ported to ctypes

The Cython-bindings provided a ".void_pointer" attribute, ctypes does
not. With ctypes, then objects which are pointer-types, can be used as
pointer-types. However, for comparison, then these need to be cast to
void-pointer, which is why the helper pointer_equal() is used.

refactor(py/tests/buf): ported to ctypes

Nothing out of the ordinary.

refactor(py/tests/enum): ported to ctypes

In addition to porting, then the module is extended with a test moved
from "test_backend.py" where the device is opened before enum is
invoked. Seemed better to place the enumeration tests in the same place.

refactor(py/tests/kv): port to ctypes

* Replacing xnvmec_perr() with os.strerr(), since ctypes-bindings do not
  provide the xnvmec_* API
* Replacing xnvme.xnvme_void_p(buf_key.void_pointer) with buf_key
  - This is an area where the ctypes-bindings is a bit nicer
* Renamed buffers, and _size to _nbytes
  - buf     to vbuf
  - buf_key to kbuf

refactor(py/tests/io): add tests of basic sync. I/O

These are new tests verifying that basic read/write via the xNVMe Python
Bindings work as intended.

refactor(py/tests/linux_hugepage): skipped (partially ported)

The various hugepage related helpers are added to the test-file
additionally, then the tests from test_backends.py are moved into this
as well since they exercised the hugepage related code as well.

refactor(py/tests/lblk): skip lblk tests

These are not-yet ported, instead a skip is added such that one can just
thrown pytest at the python/tests folder, and eventually port tests
currently not ported.

refactor(py/tests/utils): replace utils.

This provides a couple of utilities:

* xnvme_cmd_ctx_cpl_status(), since this is a "static inline" function
  in the C API, then the auto-generated bindings does not provide it.

* pointer_equal(), checks whether two ctypes pointer-objects point to
  the same memory

* dev_from_params(), constructs Python handle to a device using
  dict-args. provided by xnvme_parametrize()

* buf_and_view(), helper allocating buffer as well as numpy-view, the
  numpy-view is a convenient way of doing array-programming operations
  in a concise manner.

feat(cijoe): add python tests to testing on Debian

Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
@safl safl mentioned this pull request Jun 5, 2023
49 tasks
@safl safl merged commit da7f99f into xnvme:next Jun 5, 2023
32 checks passed
karlowich added a commit to karlowich/xnvme that referenced this pull request Jun 7, 2023
Signed-off-by: Karl Bonde Torp <k.torp@samsung.com>
safl pushed a commit that referenced this pull request Jun 7, 2023
Signed-off-by: Karl Bonde Torp <k.torp@samsung.com>
safl pushed a commit that referenced this pull request Jun 20, 2023
Signed-off-by: Karl Bonde Torp <k.torp@samsung.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-ready-for-test PR is ready for functional tests
Development

Successfully merging this pull request may close these issues.

None yet

1 participant