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.

Sign up for GitHub

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

Jump to bottom

Use python's setup.py to compile & install monitor #2

Merged
merged 2 commits into from
Oct 31, 2022
Merged

Use python's setup.py to compile & install monitor #2

merged 2 commits into from
Oct 31, 2022

Conversation

doronbehar
Copy link

@doronbehar doronbehar commented Oct 9, 2022 •
edited
Loading

Find monitor shared object file using a relative path to __file__.

@doronbehar
Copy link
Author

@bleykauf in order to address the ideas suggested in linien-org/linien#294 and linien-org/linien#277 (comment), this will help a lot. The current version of this PR makes installation of pyrp3 as simple as

pip install git+https://github.com/linien-org/pyrp3

I tested the importing PyRedPitaya with this change.

@bleykauf
Copy link

@doronbehar: You tested this on a fresh installation of the image? Otherwise libmonitor.so might still be present from a previous manual installation.

@bleykauf bleykauf changed the base branch from master to develop October 26, 2022 06:15
@bleykauf
Copy link

Also, apparently I did not set up a pre-commit file, and thus CI fails.

Also changed base branch to develop.

@doronbehar
Copy link
Author

You tested this on a fresh installation of the image?

On my x86_64 machine, I installed it on a fresh python environment with all 4 dependencies installed as well, and I tested that I can import PyRedPitaya.

@bleykauf bleykauf self-requested a review October 26, 2022 06:26
bleykauf
bleykauf reviewed Oct 26, 2022
View reviewed changes
Copy link

@bleykauf bleykauf left a comment

Choose a reason for hiding this comment

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

Only a quick first look at this with some requests for some changes.

I only had time for a brief look but I don't undertand where libmonitor is compiled.

I also realized that there are two unmerged feature branches that I left abandoned. I will merge them now which will probably result in merge conflicts that we will have to resolve. Sorry for that!

PyRedPitaya/raw_memory.py Outdated
@@ -3,11 +3,25 @@
from ctypes import *
import numpy as np
from time import time
Copy link

Choose a reason for hiding this comment

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

Import is unused

PyRedPitaya/raw_memory.py Outdated Show resolved Hide resolved
PyRedPitaya/raw_memory.py Outdated
@@ -3,11 +3,25 @@
from ctypes import *
import numpy as np
from time import time
from os import path
import platform

if 'PyRedPitayaTest' in list(sys.modules.keys()):
from PyRedPitayaTest import libmonitor_file
Copy link

Choose a reason for hiding this comment

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

Looks like a relict from earlier version.

Copy link

Choose a reason for hiding this comment

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

Talking about PyRedPitayaTest

setup.py Show resolved Hide resolved
@bleykauf
Copy link

Merged #3

As expected, there are some merge conflicts...

@bleykauf
Copy link

@doronbehar I think applying your changes to the current state of the develop branch and rebasing could be the easiest way to resolve this?

@bleykauf
Copy link

@doronbehar: Already resolved in github's editor. Thought this would automatically merge.

@bleykauf
Copy link

Currently raw_memory.py is missing import sys, not sure if I accidentally deleted it.

bleykauf
bleykauf requested changes Oct 26, 2022
View reviewed changes
Copy link

@bleykauf bleykauf left a comment

Choose a reason for hiding this comment

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

OK, now I understand where libmonitor is compiled. Did not know about Extension before :)

PyRedPitaya/raw_memory.py Outdated Show resolved Hide resolved
PyRedPitaya/raw_memory.py Outdated Show resolved Hide resolved
PyRedPitaya/raw_memory.py Outdated Show resolved Hide resolved
@doronbehar
Copy link
Author

I rebased this myself onto develop and pushed and it seems fine according to the same test as before.

@doronbehar
Copy link
Author

With this change, putting pyrp3 on Pypi will allow us to get rid of handling it in linien_install_requirements.sh. along with systemd instead of screen, and making mdio-tool part of linien will make linien_install_requirements.sh not required anymore.

@doronbehar doronbehar changed the title Use correct prefixes in setup.py raw_memory.py Use python's setup.py to compile & install monitor Oct 26, 2022
bleykauf
bleykauf requested changes Oct 28, 2022
View reviewed changes
PyRedPitaya/raw_memory.py Outdated Show resolved Hide resolved
@doronbehar
Use python's setup.py to compile & install monitor
b5b0dbe 
Find monitor shared object file using a relative path to `__file__`.
@bleykauf
Copy link

There are now again conflicts within this branch. Please don't force-push, makes everything pretty messy.

@bleykauf bleykauf merged commit bd6ff3e into linien-org:develop Oct 31, 2022
@bleykauf
Copy link

Rebased and merged. Will change the open issues from the review separately.

@doronbehar doronbehar mentioned this pull request Mar 1, 2023
Merged
@doronbehar doronbehar mentioned this pull request Aug 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bleykauf bleykauf bleykauf requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@doronbehar @bleykauf