[EN-3260] [PythonAPI] Add compatibility of PythonAPI with Python 3.9+ #39

ttldtor · 2021-08-28T18:19:23Z

No description provided.

…le names

…st on PR

asalynskii

Looks really good 👍 , there are few places I would like to ask to change a bit, but they are more about cosmetic ones or our usual approaches in terms of solving some regular cases.

For example - usage of pathlib as standart library to handle things about interaction with file system. Doing so we are trying to focus our team experience and skills on some particular set of technologies to achieve excellence in.

asalynskii · 2021-08-31T12:39:21Z

.github/workflows/publishpackage.yml

@@ -34,12 +34,11 @@ jobs:
          poetry install --no-root
        shell: bash
      - name: Create tarball
-        if: matrix.os == 'ubuntu-latest' && matrix.python-version == 3.7
+        if: matrix.os == 'ubuntu-latest' || matrix.os == 'ubuntu-18.04'


This means that we'll run this task 8 times in a row, but in reality we need only one tarball with source distribution. Please consider variant to create only one of them. ;)

You're right, yes.

asalynskii · 2021-08-31T12:42:37Z

.github/workflows/publishpackage.yml

@@ -11,8 +11,8 @@ jobs:
  build:
    strategy:
      matrix:
-        os: [ubuntu-latest, windows-latest, macos-latest]
-        python-version: [3.6, 3.7, 3.8]
+        os: [ubuntu-18.04, ubuntu-latest, windows-latest, macos-latest]


This looks just a bit strange, why do we have such priority for Bionic Beaver (18.04)? So according to this configuration we are building for ubuntu 21.04/21.10 and 18.04. Why so? This question is actual for this config and other ones with same line as well.

What is the reason for choosing the minimum Linux version? This is because glibc "controls" everything. There is some backward compatibility. That is, we can take Ubuntu 18.04 (this is LTS) and get the minimum version of glibc (2.27 https://launchpad.net/ubuntu/bionic/+source/glibc ). The so libraries will be able to run on newer versions of glibc.

By the way, ubuntu-latest is Ubuntu 20.04 LTS. https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources.

I suggest using the minimal version for wheel too. Or all versions.

Oh, okay, this sounds pretty reasonable. If we can rely on ubuntu's backward comp and we can use all necessary python versions with it - I have no objections to use 18.04 as base version for linux build. 👍 🙂

asalynskii · 2021-08-31T12:49:50Z

build.py

+        __c_api_extracted_root_dir = self.__path_to_extract / f'dxfeed-c-api-{self.__version}-no-tls'
+        is_x64 = 8 * struct.calcsize("P") == 64
+
+        if platform.system() == 'Windows':


We are usually using default values approach instead of additional elses. It's not something critical, just our preferable way of resolving such cases. Actual here and in following if/else statements as well.

asalynskii · 2021-08-31T13:01:53Z

build.py

+
+        __c_api_library_name = f'DXFeed{c_api_debug_suffix}{c_api_x64_suffix}'
+
+        if platform.system() == 'Windows':


Probably if we'll reformat this 2 level if swapping platform's instruction set (x86/x64) and OS name then we'll be able to reuse some part of of the logic and avoid part of the code duplications.

I rewrote this. Hopefully it's gotten a little shorter.

asalynskii · 2021-08-31T13:13:03Z

build.py

+                c_api_extracted_library_dir = __c_api_extracted_root_dir / 'bin' / 'x64'
+                c_api_library_file_name = f'lib{__c_api_library_name}.dylib'
+            else:
+                raise Exception('Unsupported platform')


I would call Exception too general class for this purposes. What do you think about using i.e. RuntimeError? 🤔

asalynskii · 2021-08-31T14:11:35Z

pyproject.toml

+sphinx = '^4.1.2'
+sphinx_rtd_theme = '^0.5.2'
+pygments = '^2.10'
+toml = '^0.10.2'


We already have toml as usual dependencies, so no need to add it to dev-deps too. 😉

asalynskii · 2021-08-31T14:19:51Z

build.py

+
+    def download(self) -> str:
+        __c_api_extracted_root_dir = self.__path_to_extract / f'dxfeed-c-api-{self.__version}-no-tls'
+        is_x64 = 8 * struct.calcsize("P") == 64


We are usually using single quotes to indicate character or str values. It's just internal team arrangement. 🙂 Actual here and in other cases as well.

asalynskii · 2021-08-31T14:21:32Z

build.py

+            print(f'Extracting to "{self.__path_to_extract}"')
+            zipfile.extractall(self.__path_to_extract)
+
+        if __debug__:


Can you please add some explanation here about how it could and should be used in the future? 🤔

I mean here not exactly __debug__ field, but use cases for this particular case. 🤔

asalynskii · 2021-08-31T18:14:38Z

build.py

+
+        if (not os.path.exists(self.__path_to_extract)) or (not os.path.exists(__c_api_extracted_root_dir)):
+            url = f'https://github.com/dxFeed/dxfeed-c-api/releases/download/{self.__version}/dxfeed-c-api-' \
+                  f'{self.__version}-{current_os}-no-tls.zip '


Looks like here is one redundant space on the line's end. 😉

I would suggest to move github link to the pyproject.toml file same way as we have moved version there 🤔

pyproject.toml

ttldtor added 26 commits August 28, 2021 18:37

[EN-3260] Download api 8.3.0. Fix the listeners & pxd_include

31e04f8

[EN-3260] remove submodule

20693f1

[EN-3260] fix build, clear tests

fdc9790

[EN-3260] fix build, clear tests

33430cd

[EN-3260] extra compiler args

06253f3

[EN-3260] extra compiler args

38e1591

[EN-3260] rpath

657f1e4

[EN-3260] rpath

9ee8ec3

[EN-3260] runtime_library_dirs

5ffc9e0

[EN-3260] runtime_library_dirs

158c117

[EN-3260] runtime_library_dirs

41f7922

[EN-3260] runtime_library_dirs

49df79c

[EN-3260] runtime_library_dirs + @loader_path

ddc871a

[EN-3260] install_name_tool

a40e186

[EN-3260] install_name_tool

1aff4e2

[EN-3260] install_name_tool

57707c3

[EN-3260] install_name_tool

826c38b

[EN-3260] @executable_path

d38c9bf

[EN-3260] install_name_tool

d2a89d1

[EN-3260] test 8.3.1

0c6adec

[EN-3260] test 8.3.1

6b7da67

[EN-3260] test 8.3.1

248d600

[EN-3260] test 8.3.1

ab5cb6c

[EN-3260] -runtime_library_dirs

0720ff7

[EN-3260] fix get_include(), fix c api version retrieving, fix the fi…

70f2936

…le names

[EN-3260] add Downloader, build.native-dependencies, ubuntu-18.04, te…

1c8eb4d

…st on PR

ttldtor requested a review from asalynskii August 30, 2021 12:49

[EN-3260] -debug version

f80070b

asalynskii requested changes Aug 31, 2021

View reviewed changes

[EN-3260] fix clear

bbb136b

ttldtor added 7 commits September 3, 2021 13:12

[EN-3260] fix workflows

04211d9

[EN-3260] docs, comments, fixes

27e18a1

[EN-3260] version 0.5.3

3b704e5

[EN-3260] toml and pandas version

898f2b2

[EN-3260] pandas version

f90440f

[EN-3260] pandas version

82746a1

[EN-3260] pandas version

0610ece

ttldtor requested a review from asalynskii September 3, 2021 18:39

asalynskii approved these changes Sep 3, 2021

View reviewed changes

This was linked to issues Sep 3, 2021

cannot build/install dxfeed 0.5.2 on python 3.9.5/3.9.6 #37

Closed

Update dev dependencies to pandas ^1.0.0 #36

Closed

asalynskii merged commit 26c4f78 into dxFeed:master Sep 3, 2021


		__c_api_library_name = f'DXFeed{c_api_debug_suffix}{c_api_x64_suffix}'

		if platform.system() == 'Windows':

[EN-3260] [PythonAPI] Add compatibility of PythonAPI with Python 3.9+ #39

[EN-3260] [PythonAPI] Add compatibility of PythonAPI with Python 3.9+ #39

Uh oh!

Conversation

ttldtor commented Aug 28, 2021

Uh oh!

asalynskii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asalynskii Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

asalynskii Sep 1, 2021 •

edited

Loading