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

Refactor packaging #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

adriendelsalle
Copy link

@adriendelsalle adriendelsalle commented Jul 5, 2022

Description

Refactor packaging:

  • add a dedicated package structure for gbs(-core) and extensions
    • create a gbs::core target for main lib, gbs::<ext> for extensions
    • add a CMake configs
  • refactor headers installation to get core and ext libs installing in <prefix>/include/gbs
    • core lib installing directly in <prefix>/include/gbs
    • ext libs installing in <prefix>/include/gbs/<ext> (e.g. <prefix>/include/gbs/mesh)
    • refactor includes according to this change, and remove unnecessary include_directories
  • refactor CMakeLists.txt files for reflect dependencies (includes and linked libs)
    • remove unnecessary link_directories
  • fix build errors
    • add <cstddef> in gbslib.h
  • refactor Python bindings to install in Python sources
  • update win and unix conda recipe scripts (not tested yet)

Note: this is a massive PR I would be happy to discuss
Note: there is no change in project implementation

@ssg-aero
Copy link
Owner

ssg-aero commented Jul 5, 2022

First of all, let me tell you I appreciate your efforts.
I agree project structure can be improved.
But it turns out impossible to make a code review.
Some files are modified then moved, so diff are hard to track.

Can we process step by step with a talk about priority first ?

@ssg-aero
Copy link
Owner

ssg-aero commented Jul 5, 2022

I'm puzzled by the issue with std::size_t, during last build for Linux I didn't notice anything although I used gcc12

@ssg-aero
Copy link
Owner

ssg-aero commented Jul 6, 2022

Maybe we should consider adding in gbs/gbslib.h inside gbs namespace using size_t = std::size_t; ?

@elac-safran
Copy link

This seems reasonnable to me, since size_t is fairly standard.
If you are to include it in a top-level header, though, I would advice creating a dedicated header file types.h, say, that would be included in gbslib.h.

@ssg-aero
Copy link
Owner

ssg-aero commented Jul 6, 2022

This seems reasonnable to me, since size_t is fairly standard. If you are to include it in a top-level header, though, I would advice creating a dedicated header file types.h, say, that would be included in gbslib.h.

moreover some types are already defined here, so moving them into a specific place could be a good idea

@adriendelsalle
Copy link
Author

I'm puzzled by the issue with std::size_t, during last build for Linux I didn't notice anything although I used gcc12

outchy, maybe I missed something then, a C header or something let me have a look

Maybe we should consider adding in gbs/gbslib.h inside gbs namespace using size_t = std::size_t;

Sounds good in gbs namespace

moreover some types are already defined here, so moving them into a specific place could be a good idea

let's do that!

First of all, let me tell you I appreciate your efforts.
I agree project structure can be improved.
But it turns out impossible to make a code review.
Some files are modified then moved, so diff are hard to track.
Can we process step by step with a talk about priority first ?

Totally, sorry for this huge PR. When it comes to package refactoring there are a LOT of changes and it's quite impossible to review. Let's schedule a chat when possible!

@adriendelsalle
Copy link
Author

just including <cstddef> did the trick for size_t, I'll rework this PR to remove renaming and reduce changes

add a dedicated package structore for gbs(-core) and extensions
create a gbs::core target for main lib
create extension targets for extensions libs
refactor CMakeLists.txt files for reflect dependencies
add missing headers for size_t
refactor Python bindings to install in Python sources
add missing forward decl of make_surface
@adriendelsalle
Copy link
Author

adriendelsalle commented Jul 6, 2022

I just forced pushed to:

  • remove renaming of size_t in std::size_t
  • remove auto formatting of tests (sorry for that I missed that my clang-format install did it!)
  • break in 3 commits/steps the refactoring

Most changes are about CMakeLists.txt files and moving headers in sudirs to reflect the include tree. I hope it looks better now!

add cmake configs for mesh, io, occt and render extensions
@adriendelsalle adriendelsalle marked this pull request as ready for review July 6, 2022 09:49
@elac-safran
Copy link

just including <cstddef> did the trick for size_t, I'll rework this PR to remove renaming and reduce changes

Now that you mention it, that is indeed what I usually do.

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.

None yet

3 participants