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

Add support for Windows (MSVC) #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rettenbs
Copy link

Prerequirements
MSVC (only Build Tools), CMake, Ninja

Steps to build
All commands have to be executed in a "MSVC Native Tools Command Prompt" to get the compiler settings.

  • Download or clone fmt

  • Create a build directory for fmt and go to the build directory

  • set CC=CL

  • set CXX=CL

  • cmake -G Ninja -DCMAKE_INSTALL_PREFIX=path\to\fmt\installation -DCMAKE_BUILD_TYPE=Release path\to\fmt\source

  • ninja install

  • set CMAKE_PREFIX_PATH=path\to\fmt\installation

  • set CMAKE_GENERATOR=Ninja

  • pip install --user -e path\to\dltpy\source

Notes

@rettenbs rettenbs mentioned this pull request Jun 10, 2020
Copy link
Owner

@Equidamoid Equidamoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you a lot for the contribution! I would've never fix it myself!

There are, however, some minor things worth changing IMO. Please see the comments.

import os
import sys
import pathlib
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pathlib. everywhere is nothing but redundant. In this context we don't have anything even coming close to have name Path ambiguous.

bin_dir = os.path.join(build_dir, 'dltpy', 'native')
self.distribution.bin_dir = bin_dir

pyd_path = [os.path.join(bin_dir, _pyd) for _pyd in
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use pathlib? It should make this part much shorter

bin_dir = os.path.join(build_dir, 'dltpy', 'native')
self.distribution.bin_dir = bin_dir

pyd_path = [os.path.join(bin_dir, _pyd) for _pyd in
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, pyd is a pretty weird name for a variable containing a path to a library.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its probably because the libraries have the ending .pyd on Windows.

if not self.dry_run:
self.spawn(['cmake', '--build', '.'] + build_args)
os.chdir(str(cwd))
self.spawn(['cmake', '-H'+str(pathlib.Path().absolute()), '-B'+self.build_temp,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This .Path() looks a bit weird. Why is it needed?


# Now that the necessary directories are created, build

self.announce("Configuring cmake project", level=3)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice logs =)

fmt::fmt
pybind11::pybind11
)

if(WIN32)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried using the pybind11_add_module macro from pybind11? https://pybind11.readthedocs.io/en/master/compiling.html#pybind11-add-module

It could be that it will solve the problem without adding platform-specific code here (I'd try it myself, but can't test it for windows at all...).

@@ -80,15 +78,15 @@ DltReader::DltReader(bool expectStorage)

std::string DltReader::str(){
return fmt::format("DltReader(buf={:p}, msg={:p}, data end={:p})",
iBuffer.begin(),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh man, indeed, the iterators are not guaranteed to be pointers. I wonder if some linter could've found this (it's not even a warning on linux&osx).

@rettenbs
Copy link
Author

I took the code for the new setup.py from here: https://stackoverflow.com/questions/42585210/extending-setuptools-extension-to-use-cmake-in-setup-py. I was happy getting it running and did not look into the details. The main issue was, that the library was not copied to the correct location. I am not sure if the complete rewrite is actually necessary.

Unfortunately, I have only limited time for this project at the moment. I will try to come back as soon as possible. If you want to fix things yourself, you can push to my repository to update this pull request.

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