Fix make #24
Fix make #24
Conversation
without this installation causes a warning on conda environments and an error on pip venvs
these dependencies neither seem to exist nor are they seem to be required
a proper `CONTRIBUTING.md` , style guide, precommit file would be hlepful as well
| @@ -23,7 +23,7 @@ setup: | |||
| $(shell "${PROTOBUF_SETUP}") | |||
| $(shell "${OPENSSL_SETUP}") | |||
| which cargo || curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y | |||
| which maturin || pip install maturin | |||
| which maturin || pip install maturin[patchelf] | |||
There was a problem hiding this comment.
should this optional dependency be in pyproject.toml too?
There was a problem hiding this comment.
I think that would be a good idea. I'll check how setting it up in venv operates when it is included in pyproject.toml
There was a problem hiding this comment.
Documentation regarding patchelf only mentions installing it with maturin itself with
pip install maturin[patchelf]
https://www.maturin.rs/installation
https://www.maturin.rs/distribution
This leads me to believe the way i did it is the way to go without further changing pyproject.toml itself
I'm looking into other projects using maturin with patchelf to see how other packages handle it.
There was a problem hiding this comment.
https://github.com/search?q=maturin%5Bpatchelf%5D&type=code
It seems different projects approach it differently.
dolma is packaged and built with make files so it is necessary to be in the Makefile but i think having it in pyproject.toml as well can help with documentation and potential build alternatives as well.
I'll try to find the best way to implement it and add a commit.
There was a problem hiding this comment.
Added commit 2141d07
This path reflects that dolma relies on patchelf because maturin does. Also this way we use maturin's specific optional dependency of patchelf.
i'm trying to develop/contribute on my local environment and i am encountering some issues, so i am making PRs for my workaround/fixes