-
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
Basic project structure setup #17
base: main
Are you sure you want to change the base?
Conversation
@nicklafarge We can start setting up the project structure here. I'll need help with some of the python stuff, so if you want to branch of and merge into this branch to work on that, we can. |
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.
Looking good so far!
I'm not sure if we'll need an __init__.py
in the pyforest
directory since I'm not sure if there will be any python files there, or just the pybind11 C++ files
# Abseil | ||
# ---------------------------------------------------------------------------------------- | ||
FetchContent_Declare( | ||
abseil |
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.
Is this just for general use across the project, or do you have something specific in mind?
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.
Googletest can use it and it offers a better hash map implementation. I can remove it, though.
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.
Works for me - was mostly just curious what you had in mind.
@@ -1,32 +1,28 @@ | |||
# Prerequisites | |||
*.d |
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.
Why did the ignored C++ parts get removed?
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.
I removed them because all of those should be in the build directory which is ignored. We can add them back, but I figured I'd try to slim it down as much as possible
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.
Not totally sure how the Python build process will work yet, but will that be true for the dynamic libraries too? I had assumed that file would be copied into the python directory upon building, but I'm not sure on the pybind11 build details yet. I guess we can leave it out and add it back in later if a need arises
Should we include Python unit testing infrastructure as a part of this structure? @noahsadaka @rjpower4 @DhruvJ22 |
I think this comes back to the user installation story for the python library. In my mind, and this may be wrong, we need to ship an installable egg or whatever python uses. Furthermore, we should allow if required in the future to ship some python code along with it and not presuppose that we are only shipping a shared object. Again, I am out of my comfort zone here, so am open to feedback @nicklafarge @DhruvJ22 @noahsadaka |
As for the user story, it looks like pybind11 has this handy build systems page and an associated example project that I think we can use some elements from in eventually setting up the installation process and If we want to plan for the potential of having additional functionality written in directly in python, we'll have to take some extra steps to import the c++ functionality into that python module we are creating to avoid a If we do want to go down that path right out of the gate, maybe we can do a setup like this project? Or we could just leave it to C++ for now, and add that functionality later if/when we need it. What do you think @noahsadaka @DhruvJ22 ? |
3f11b9d
to
07df045
Compare
I support the second option to reduce the current complexity of the code. I don't think it will be a huge challenge to refactor the code in the future to add the capability when needed. |
CMakePresets.json