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

Support Python on Windows #67

Open
mjmdavis opened this issue Nov 30, 2022 · 6 comments · Fixed by #68
Open

Support Python on Windows #67

mjmdavis opened this issue Nov 30, 2022 · 6 comments · Fixed by #68

Comments

@mjmdavis
Copy link
Contributor

I'd like to use pyroscope for Python on Windows.

@mjmdavis
Copy link
Contributor Author

mjmdavis commented Dec 1, 2022

So, I have a windows build running. I will test it later today.

I have two platform dependent errors that I had to work around:

Process ID is platform dependent

The pid type is set to i32 in pyroscope-rs:
https://github.com/pyroscope-io/pyroscope-rs/blob/66b592b83ca245ed041c2160fada4595fcdf4a4d/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L32
https://github.com/pyroscope-io/pyroscope-rs/blob/66b592b83ca245ed041c2160fada4595fcdf4a4d/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L69

However, in py-spy, the pid type can be either i32 or u32 depending on the platform it's built on. This causes a type error when compiling pyroscope-rs for windows.

The source of this is a transitive dependency via benfred/py-spy on benfred/remoteprocess:
https://github.com/pyroscope-io/pyroscope-rs/blob/66b592b83ca245ed041c2160fada4595fcdf4a4d/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L251-L255
https://github.com/benfred/py-spy/blob/538a2c63327adc9c0b921e1502c6e9156b1e14bb/src/config.rs#L22
https://github.com/benfred/py-spy/blob/538a2c63327adc9c0b921e1502c6e9156b1e14bb/src/config.rs#L2

The Pid type returned by remoteprocess for unixy platforms is i32, defined in rust-lang/libc via this chain:
https://github.com/benfred/py-spy/blob/538a2c63327adc9c0b921e1502c6e9156b1e14bb/src/config.rs#L2
https://github.com/benfred/remoteprocess/blob/9ca4be545f46014412f86827c87589cebc621ae1/src/osx/mod.rs#L14
https://github.com/rust-lang/libc/blob/15d27952bfa93e5e4f419c603f275486f15a050c/src/unix/mod.rs#L25

For windows it's u32 on this chain:
https://github.com/pyroscope-io/pyroscope-rs/blob/66b592b83ca245ed041c2160fada4595fcdf4a4d/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L254
https://github.com/benfred/py-spy/blob/538a2c63327adc9c0b921e1502c6e9156b1e14bb/src/config.rs#L2
https://github.com/benfred/remoteprocess/blob/9ca4be545f46014412f86827c87589cebc621ae1/src/windows/mod.rs#L12
https://github.com/rbspy/read-process-memory/blob/d0aeb5088ec5df2b383df02f8772a33b484f1f81/src/lib.rs#L396
https://github.com/retep998/winapi-rs/blob/0.3/src/shared/minwindef.rs#L20
https://github.com/retep998/winapi-rs/blob/2f76bdea3a79817ccfab496fbd1786d5a697387b/src/lib.rs#L47

libc::pthread_self() is not portable to windows

Less complicated to fix this function but perhaps more complicated to get the functionality to work.

https://github.com/pyroscope-io/pyroscope-rs/blob/70ff908cb84a4b15fefcf52275c1ca1bbb72569a/src/utils.rs#L76-L80

@mjmdavis
Copy link
Contributor Author

mjmdavis commented Dec 1, 2022

@mjmdavis
Copy link
Contributor Author

mjmdavis commented Dec 1, 2022

For the pid type issue, there are a few options. For now, I added a direct dependency on remoteprocess to pyroscope_pyspy:
https://github.com/pyroscope-io/pyroscope-rs/blob/d03601a25cb2daab515303dcbdd650643c458c95/pyroscope_backends/pyroscope_pyspy/Cargo.toml#L21

This allows the use of the type determined at build time in that library:
https://github.com/pyroscope-io/pyroscope-rs/blob/d03601a25cb2daab515303dcbdd650643c458c95/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L17
https://github.com/pyroscope-io/pyroscope-rs/blob/d03601a25cb2daab515303dcbdd650643c458c95/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L70
https://github.com/pyroscope-io/pyroscope-rs/blob/d03601a25cb2daab515303dcbdd650643c458c95/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L68-L75

An alternative might be to get this type from read-process-memory which is already used in remoteprocess. However, remoteprocess only gets the pid type from there for windows builds. For other platforms, it seems to use libc directly instead of using read-process-memory. One might argue that it should use read-process-memory for consistency.

Another alternative would be for py-spy to re-export the Pid type so that extra dependencies are not needed.

@mjmdavis
Copy link
Contributor Author

mjmdavis commented Dec 1, 2022

Well, it seems that py-spy does re-export the Pid type. So I'm a dummy.

@mjmdavis
Copy link
Contributor Author

mjmdavis commented Dec 1, 2022

So, it works. I was able to send metrics to the pyroscope cloud using an example program.

korniltsev pushed a commit that referenced this issue Jan 16, 2023
* added workflow_dispatch to ci-ffi-python

* added windows build

* commented out all other builds

* set correct windows py-platform

* removed windows server 2011

* Adeed capability to get windows thread id.

* Added winapi

* bumped workflow file to trigger build

* wrapped the function with unsafe and the error checker

* made pyroscope_ffi tomls point to a path

* Used the pid type from remoteprocess to accound for platform differences.

* bump workflow

* put back the different platform python builds

* Added the windows-test file.

* Corrected a typo.

* added a windows test build

* added powershell for installing wheel

* udpated trigger on test job

* updated powershell expression

* updating main test file

* use the re-exported Pid type instead of directly depending on remoteprocess

* remove the remoteprocesd dep

* removed unnecessary Cargo change

* Added windows CI PySpy backend.

Co-authored-by: Michael Davis <mdavis@psiquantum.com>
@korniltsev korniltsev reopened this Jan 16, 2023
@korniltsev
Copy link
Collaborator

korniltsev commented Jan 16, 2023

I am having trouble to make tests working on windows

 2023-01-16T08:05:49.106Z INFO  Pyroscope::Session > Creating Session
 2023-01-16T08:05:49.106Z INFO  Pyroscope::Session > Creating Session
 2023-01-16T08:05:49.106Z INFO  Pyroscope::Session > Creating Session
 2023-01-16T08:05:49.106Z INFO  Pyroscope::Session > Creating Session
 2023-01-16T08:05:49.107Z INFO  py_spy::python_spy > Found libpython binary @ C:\hostedtoolcache\windows\Python\3.10.9\x64\python310.dll
 2023-01-16T08:05:49.111Z INFO  py_spy::python_spy > Getting version from python binary BSS
 2023-01-16T08:05:49.111Z INFO  py_spy::python_spy > Failed to get version from BSS section: failed to find version string
 2023-01-16T08:05:49.111Z INFO  py_spy::python_spy > Getting version from libpython BSS
 2023-01-16T08:05:49.112Z INFO  py_spy::version    > Found matching version string '3.10.9 (tags/v3.10.9:1dd9be6, Dec  6 2022, 20:01:21) [MSC v.1934 64 bit'
 2023-01-16T08:05:49.112Z INFO  py_spy::python_spy > python version 3.10.9 detected
 2023-01-16T08:05:49.112Z INFO  py_spy::python_spy > got symbol _PyRuntime (0x00007ffa60f3e000) from libpython binary
 2023-01-16T08:05:49.112Z WARN  py_spy::python_spy > Interpreter address from _PyRuntime symbol is invalid 003cb6e000173831
 2023-01-16T08:05:49.112Z INFO  py_spy::python_spy > Failed to get interp_head from symbols, scanning BSS section from main binary
 2023-01-16T08:05:49.113Z INFO  py_spy::python_spy > Failed to get interpreter from binary BSS, scanning libpython BSS
 2023-01-16T08:05:22.606Z INFO  py_spy::python_spy > Failed to connect to process, retrying. Error: Failed to find a python interpreter in the .data section

I am trying the test from the branch #76 , but on master it also does not work

The job https://github.com/pyroscope-io/pyroscope-rs/actions/runs/3928207781/jobs/6715641552

@mjmdavis Would you like to take a look? If no, I will try to do it later some time.

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 a pull request may close this issue.

2 participants