updates and fixes for windows build by timkpaine · Pull Request #884 · perspective-dev/perspective

timkpaine · 2020-01-28T04:46:23Z

A few updates and fixes for the windows build of perspective-python

build dlls and pyd file
set prefix for assets to lib for symmetry
inline arrow dlls and tbb dll
omit tests using missing platform functions
swap slashes in setup.py for windows paths
change .appveyor.yml to match travis

Still unresolved:
https://bugs.python.org/issue36439

timkpaine · 2020-02-02T03:00:03Z

scripts/script_utils.js

-    [resolve`${resolve`${__dirname}/../python/perspective/dist`}/cmake`, _path.resolve(_path.resolve(__dirname, "..", "python", "perspective", "dist"), "cmake")],
-    [resolve`${resolve`${__dirname}/../python/perspective/dist`}/obj`, _path.resolve(_path.resolve(__dirname, "..", "python", "perspective", "dist"), "obj")]
-]);
+if (isWin){


@texodus can you take a look at this, the comments seem to suggest that this was supposed to work but it does not on windows

timkpaine · 2020-02-02T03:02:38Z

cpp/perspective/CMakeLists.txt

 include_directories("${CMAKE_SOURCE_DIR}/src/include")

+if(NOT WIN32)
+	set(CMAKE_CXX_FLAGS_RELEASE "-O3 -DNDEBUG")


@sc1f note this

timkpaine · 2020-02-02T03:03:04Z

cpp/perspective/CMakeLists.txt

-			")
+	if(WIN32)
+		if(CMAKE_BUILD_TYPE_LOWER STREQUAL debug)
+			set(OPT_FLAGS " \


@sc1f we should set debug flags (see prev comment) instead of having this opt flags thing

timkpaine · 2020-02-02T03:04:42Z

cpp/perspective/CMakeLists.txt

-	    endif()
+		endif()
+	else()
+		set(CMAKE_SHARED_LIBRARY_PREFIX lib)


@sc1f we should consider not prefixing with lib, and setting the prefixes to "" (this will mean we need to change python imports from libbinding to just binding, or preferable _binding)

timkpaine · 2020-02-02T03:06:38Z

cpp/perspective/CMakeLists.txt

+
+		if(WIN32)
+			# inline arrow dlls
+			file(GLOB ARROW_DLLS "${PYTHON_PYARROW_LIBRARY_DIR}/*.dll")


@sc1f @texodus open to suggestions to get around copying all arrow dlls into our directory. In a "production" environment with a custom arrow build you won't have to worry about this, but with the packaged arrow wheel bringing a ton of stuff with it, we need to ensure all the links resolve.

I don't understand what the requirement being solved by copying is, nor why a ton of stuff being brought is the instigator, so I'm not really in a position to offer constructive advice here. Copying is bad of course.

timkpaine · 2020-02-02T03:07:18Z

cpp/perspective/src/include/perspective/scalar.h


 template <>
-std::int64_t t_tscalar::get() const;
+PERSPECTIVE_EXPORT std::int64_t t_tscalar::get() const;


@sc1f i assume all these specializations are necessary?

Yeah, they all need to be explicitly specialized.

timkpaine · 2020-02-02T03:08:04Z

cpp/perspective/src/include/perspective/sym_table.h

-const char* get_interned_cstr(const char* s);
-t_tscalar get_interned_tscalar(const char* s);
-t_tscalar get_interned_tscalar(const t_tscalar& s);
+PERSPECTIVE_EXPORT const char* get_interned_cstr(const char* s);


@sc1f this is currently being exposed as part of libpsp's public interface (it is used in the python cpp code). should this be public?

timkpaine · 2020-02-02T03:08:33Z

python/perspective/package.json

  "dependencies": {
-    "@finos/perspective": "^0.4.1"
+    "@finos/perspective": "^0.4.1",
+    "zerorpc": "^0.9.8"


@texodus please confirm

At this point I see honestly zero reason to maintain this feature - perspective-python is better in every way. I propose we delete the perspective-node binding entirely.

timkpaine · 2020-02-02T03:08:58Z

python/perspective/perspective/include/perspective/python/numpy.h

     * NumpyLoader fast-tracks the loading of Numpy arrays into Perspective, utilizing memcpy whenever possible.
     */
-    class PERSPECTIVE_EXPORT NumpyLoader {
+    class PERSPECTIVE_BINDING_EXPORT NumpyLoader {


@sc1f is a class necessary?

it follows the structure set by arrow_loader, and allows us to abstract all the numpy-specific stuff since its' all state-dependent

timkpaine · 2020-02-02T03:10:11Z

scripts/test_python.js

        // Tests for client mode (when C++ assets are not present) require
        // a new runtime.
-        execute`cd ${python_path} && TZ=UTC ${PYTHON} -m pytest \
+        process.env.TZ = "UTC";


@sc1f note for os-agnostic

texodus

Thanks for the PR! I have it on good authority that it looks good!

updates and fixes for windows build

6b0a6e4

timkpaine added enhancement Feature requests or improvements C++ Python labels Jan 28, 2020

finos-admin added the cla-present label Jan 28, 2020

timkpaine added 2 commits February 1, 2020 21:39

merge master

34baef0

fix windows path assertions

0aba39c

timkpaine commented Feb 2, 2020

View reviewed changes

revert appveyor changes

a4c4436

timkpaine commented Feb 2, 2020

View reviewed changes

timkpaine added 6 commits February 1, 2020 22:10

cleanup

b2ee0d8

attempt python symlink

a7b4cd7

attempt set python3.7 on appveyor

fe0143b

more appveyor tweaks

eba38eb

symlink take 2

82acc6a

install numpy and pyarrow

0c2518b

timkpaine mentioned this pull request Feb 2, 2020

Bug: Missing class exports on windows #883

Closed

3 tasks

slight change

b6d1457

texodus approved these changes Feb 8, 2020

View reviewed changes

texodus merged commit a89d7e4 into master Feb 8, 2020

texodus deleted the windows branch February 8, 2020 16:59

texodus mentioned this pull request Feb 8, 2020

Remove perspective.node Python module. #912

Merged

Conversation

timkpaine commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

texodus Feb 8, 2020 • 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!

texodus left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

timkpaine commented Jan 28, 2020 •

edited

Loading

texodus Feb 8, 2020 •

edited

Loading