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

implement BridgeStan download and module compilation on Rust #212

Merged
merged 32 commits into from
May 1, 2024

Conversation

randommm
Copy link
Contributor

@randommm randommm commented Feb 15, 2024

Implement BridgeStan download and module compilation on Rust.

Fixes issue #100

Copy link
Collaborator

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

Thank you for taking this on! A few thoughts on what is here so far

rust/Cargo.toml Outdated Show resolved Hide resolved
rust/src/download_compile.rs Outdated Show resolved Hide resolved
rust/tests/model.rs Show resolved Hide resolved
@randommm
Copy link
Contributor Author

Thank you for taking this on! A few thoughts on what is here so far

okay, all done

@WardBrian
Copy link
Collaborator

You may have to skip some of the compile tests on Windows for Rust, since we never unload libraries (#111) and Windows locks the file of loaded libraries

@randommm
Copy link
Contributor Author

randommm commented Feb 18, 2024

i see, what about Mac? it seems to give some weird error that is hard to understand, seems to be C++ toolchain related.

@WardBrian
Copy link
Collaborator

I’ll try to take a look this week - my guess is it has to do with the specific CI environment installing a non-native LLVM

@randommm
Copy link
Contributor Author

there also seems to be some toolchain problem on Windows, lack of mingw, so it's unable to compile the example.

Copy link
Collaborator

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

Thanks again. A few more comments below.

I'm also going to ask @aseyboldt if he would be willing to take a look at this

rust/README.md Show resolved Hide resolved
rust/.vscode/settings.json Outdated Show resolved Hide resolved
.github/workflows/main.yaml Outdated Show resolved Hide resolved
rust/src/download_compile.rs Outdated Show resolved Hide resolved
rust/src/bs_safe.rs Outdated Show resolved Hide resolved
rust/src/download_compile.rs Outdated Show resolved Hide resolved
@randommm randommm force-pushed the main branch 6 times, most recently from 6240094 to 89373cf Compare March 7, 2024 21:26
@randommm
Copy link
Contributor Author

randommm commented Mar 7, 2024

finally managed to fix the build on macos now!

@randommm
Copy link
Contributor Author

randommm commented Mar 9, 2024

@WardBrian @aseyboldt made the API a little more clear now regarding that bridgestan is being download, and also easier to point to a custom bridgestan dir. I think that the checksum and the lock dir would be better fit as others issues/PRs.

rust/Cargo.toml Outdated Show resolved Hide resolved
@randommm randommm requested a review from WardBrian March 23, 2024 09:09
Copy link
Collaborator

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

The following are hopefully my final comments, focusing on some edge cases in the example and tests.

I'd still like @aseyboldt do give another look over for any Rust nits I'm missing. But I think this is very good!

rust/README.md Outdated Show resolved Hide resolved
rust/examples/example.rs Outdated Show resolved Hide resolved
rust/tests/model.rs Outdated Show resolved Hide resolved
rust/tests/model.rs Outdated Show resolved Hide resolved
rust/src/download.rs Show resolved Hide resolved
rust/src/compile.rs Outdated Show resolved Hide resolved
@WardBrian WardBrian linked an issue Apr 5, 2024 that may be closed by this pull request
@randommm randommm requested a review from WardBrian April 6, 2024 10:29
Copy link
Collaborator

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

This looks good to me. I left two tiny questions about the tests, and I would still appreciate if @aseyboldt could have a look before merging.

Thanks again!

rust/tests/model.rs Outdated Show resolved Hide resolved
rust/tests/model.rs Outdated Show resolved Hide resolved
@WardBrian WardBrian merged commit 9ffa236 into roualdes:main May 1, 2024
19 checks passed
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 this pull request may close these issues.

Ability to build models from inside Rust
3 participants