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

Fix make #24

Merged
merged 6 commits into from
Aug 22, 2023
Merged

Fix make #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0e3707c
add maturin feature
chris-ha458 Aug 19, 2023
9b445c4
fix test-rust
chris-ha458 Aug 19, 2023
808f9e8
add style guide
chris-ha458 Aug 19, 2023
2141d07
reflect patchelf dependency
chris-ha458 Aug 22, 2023
9b709f1
Merge branch 'main' into fix_make
soldni Aug 22, 2023
ac6cf0f
Merge branch 'main' into fix_make
soldni Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Member

Choose a reason for hiding this comment

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

should this optional dependency be in pyproject.toml too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added commit 2141d07


publish:
maturin publish
Expand All @@ -32,7 +32,7 @@ test-python:
pytest -vs tests/python
rm -rf tests/work/*

test-rust: test-rust-clean test-rust-setup
test-rust:
cargo test -- --nocapture
rm -rf tests/work/*

Expand Down
6 changes: 6 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ make test

You can choose to run just the Python or Rust tests by calling `make test-python` or `make test-rust` respectively.

## Contributing

Before committing, use the following command
```shell
make style
```

## Citation

Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ dev = [
]
[build-system]
requires = [
"maturin>=1.1,<2.0",
"maturin[patchelf]>=1.1,<2.0",
"setuptools >= 61.0.0",
"wheel"
]
Expand Down