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

Basic project structure setup #17

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

rjpower4
Copy link
Member

@rjpower4 rjpower4 commented Nov 9, 2022

  • External dependencies
    • Boost
    • Googletest
    • Abseil
    • LibFMT
    • PyBind11
  • Unit tests
  • CMake
    • CMakePresets.json
    • Cross-platform warnings
    • Install and packaging targets
  • Documentation generation
  • Code Style/Guidelines
    • Clang Format
    • Clang Tidy

@rjpower4
Copy link
Member Author

rjpower4 commented Nov 9, 2022

@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.

Copy link
Member

@nicklafarge nicklafarge left a 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

CMakeLists.txt Show resolved Hide resolved
examples/CMakeLists.txt Show resolved Hide resolved
# Abseil
# ----------------------------------------------------------------------------------------
FetchContent_Declare(
abseil
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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

@nicklafarge
Copy link
Member

Should we include Python unit testing infrastructure as a part of this structure? @noahsadaka @rjpower4 @DhruvJ22

@rjpower4
Copy link
Member Author

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

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

@nicklafarge
Copy link
Member

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 setup.py. Maybe we can use the PEP 518 version of this as a starting point (requires pip 10+).

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 from pyforest import pyforest situation. So that linked post above suggests creating _pyforest.so where the __init__.py file includes from _pyforest import *, but I guess that can break down for submodules given some of the comments.

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 ?

@rjpower4 rjpower4 force-pushed the feature/9-create-project-structure branch from 3f11b9d to 07df045 Compare November 11, 2022 02:12
@DhruvJ22
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Architecture Issues related to project architecture/setup Domain: Build Tooling Issues related to the build system
Projects
None yet
3 participants