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

If we are building mlpack statically then build executable statically #2931

Merged
merged 7 commits into from Apr 30, 2021

Conversation

shrit
Copy link
Member

@shrit shrit commented Apr 24, 2021

This pull request is not related to cross-compilation.

Signed-off-by: Omar Shrit omar@shrit.me

… too

This pull request is not related to cross-compilation.

Signed-off-by: Omar Shrit <omar@shrit.me>
Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Can you also make sure the documentation is updated? 👍

@shrit
Copy link
Member Author

shrit commented Apr 25, 2021

Do you have a specific file in mind to update? for example doc/guide/build.hpp?

@rcurtin
Copy link
Member

rcurtin commented Apr 25, 2021

The original PR had some modifications to the documentation of the options in CMakeLists.txt. And yeah, we probably should also update the descriptions in README.md and doc/guide/build.hpp too. 👍

@shrit
Copy link
Member Author

shrit commented Apr 25, 2021

Exactly, the modifications related to the main CMakeLists.txt are already embedded in the autodownloader pull request #2927, sorry for that.
I will add additional one in README.md and in doc/guide/build.hpp

Signed-off-by: Omar Shrit <omar@shrit.me>
Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Awesome, thanks---this looks good to me if you can fix the couple tiny comments. :) Thanks for adding CEREAL_INCLUDE_DIR to the documentation also!

doc/guide/build.hpp Outdated Show resolved Hide resolved
src/mlpack/bindings/cli/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Also did you want to modify the README to discuss BUILD_SHARED_LIBS too?

shrit and others added 2 commits April 29, 2021 23:08
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Co-authored-by: Ryan Curtin <ryan@ratml.org>
@shrit
Copy link
Member Author

shrit commented Apr 29, 2021

@rcurtin, sorry I forget about the readme, I will add one line to declare the option 👍

Signed-off-by: Omar Shrit <omar@shrit.me>
Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Awesome, looks good to me. Just two more tiny changes. :)

README.md Outdated Show resolved Hide resolved
src/mlpack/bindings/cli/CMakeLists.txt Outdated Show resolved Hide resolved
shrit and others added 2 commits April 30, 2021 02:10
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@rcurtin rcurtin merged commit 92d57f2 into mlpack:master Apr 30, 2021
@rcurtin
Copy link
Member

rcurtin commented Apr 30, 2021

Awesome, thanks! 👍

@shrit shrit deleted the build_static_binary branch April 30, 2021 22:52
@shrit
Copy link
Member Author

shrit commented Apr 30, 2021

You are most welcome, I will open one tomorrow for the embedded system 👍

This was referenced Oct 14, 2022
@rcurtin rcurtin mentioned this pull request Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants