-
Notifications
You must be signed in to change notification settings - Fork 1
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
Move to pyproject.toml
installation.
#275
Conversation
d98d2e8
to
dc7e1ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ehpor I got curious by this so looked through it. I wanted to make sure I understand, so is it correct that this PR here does not call for immediate updates in the downstream repos, it just changes how catkit2 itself gets installed?
README.md
Outdated
@@ -96,8 +88,8 @@ conda env create --file environment.yml | |||
conda activate catkit2 | |||
``` | |||
|
|||
Finally install the package using: | |||
Finally install the package using: | |||
``` | |||
python setup.py develop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to update this to pip install -e .
as well as you're already on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in 2f361f4.
pyproject.toml
Outdated
bmc_dm = "catkit2.services.bmc_dm.bmc_dm" | ||
bmc_dm_sim = "catkit2.services.bmc_dm_sim.bmc_dm_sim" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fyi, I hope to wipe these with #285 today-ish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed during rebase.
pyproject.toml
Outdated
zwo_camera = "catkit2.services.zwo_camera.zwo_camera" | ||
|
||
[project.entry-points."catkit2.proxies"] | ||
bmc_dm = "catkit2.testbed.proxies.bmc_dm:BmcDmProxy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for the proxy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed during rebase.
Instead of setup.py.
1f4aa9a
to
37acb1b
Compare
@ivalaginja Ready for re-review. |
cd extern | ||
./download.sh | ||
cd .. | ||
``` | ||
For installation on Apple Silicon with python=3.7, you need to follow these steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind sneaking in a "first" at the end of this line, to make things clearer?
conda config --env --set subdir osx-64 | ||
conda create --name catkit2 | ||
conda activate catkit2 | ||
conda config --env --set subdir osx-64 | ||
conda env update --file environment.yml | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then maybe an "instead" at the end of line 85.
These downloads can be performed on a separate machine with internet connectivity, in case the machine on which the testbed is run does not have an (unfirewalled) internet connection. After downloading, the resulting folders can be copy-pasted in the extern folder on the machine without internet connectivity. | ||
|
||
The following will install all downloaded dependencies and create the Conda environment for catkit2. | ||
The following will install all dependencies and create the Conda environment for catkit2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, do you want to add the Py3.7 installation exception that's mentioned in the README here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just left some optional clarification comments for the docs but no need to block the PR with those. If you have a minute though... please? ^^
This PR uses Conda to install all C++ dependencies, and performs all installation (including protobuffer compilation) using CMake. This means that there is no more need to download external dependencies. I'm now relying on
scikit-build-core
to build the library, which removes a lot of the build code for CMake.