Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Install headers into manifold subdirectory #729

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Conversation

cjmayo
Copy link
Contributor

@cjmayo cjmayo commented Jan 31, 2024

Basically this goes back to the way things were. I think it addresses #701 OK, don't know if there are other ideas.

/usr/include/manifold/meshIO.h
/usr/include/manifold/sdf.h
/usr/include/manifold/manifold.h
/usr/include/manifold/polygon.h
/usr/include/manifold/cross_section.h
/usr/include/manifold/collider.h
/usr/include/manifold/vec_view.h
/usr/include/manifold/public.h

Because of the CMake config files I am able to build OpenSCAD (modified to use find_package(manifold)) with either master or this PR.

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3b8282e) 91.67% compared to head (7d01878) 91.67%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #729   +/-   ##
=======================================
  Coverage   91.67%   91.67%           
=======================================
  Files          37       37           
  Lines        4732     4732           
=======================================
  Hits         4338     4338           
  Misses        394      394           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@elalish
Copy link
Owner

elalish commented Jan 31, 2024

Thanks, sounds good to me, but I don't understand library installs well enough to review this with any confidence. If @pca006132 is happy and it works for OpenSCAD, then I'm happy.

@pca006132
Copy link
Collaborator

Looks fine to me, but ultimately I think we should change headers such as public.h to something else, or it will conflict with others...

@elalish
Copy link
Owner

elalish commented Feb 1, 2024

Can you help me understand this better? I think what's happening here is that instead of installing usr/include/public.h, which might collide with anything called public.h, we're installing usr/include/manifold/public.h, which shouldn't collide with anything except maybe another library called manifold. Or am I misunderstanding how header conflicts happen?

@pca006132
Copy link
Collaborator

Yeah this avoids conflict during installation, but if a user uses two packages with public.h, the include will have conflict as well.

@elalish
Copy link
Owner

elalish commented Feb 2, 2024

Oh, I thought you'd still have to put #include <manifold/public.h>.

@pca006132
Copy link
Collaborator

That requires changing our current directory structure and includes.

@elalish elalish merged commit 9c1517e into elalish:master Feb 5, 2024
18 checks passed
@cjmayo cjmayo deleted the include branch February 5, 2024 19:32
cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants