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

Please don't bundle admesh #1525

Closed
hroncok opened this issue Nov 12, 2013 · 16 comments
Closed

Please don't bundle admesh #1525

hroncok opened this issue Nov 12, 2013 · 16 comments

Comments

@hroncok
Copy link
Member

hroncok commented Nov 12, 2013

I can see now, that development version of Slic3r now uses admesh for repairing STLs. From the user's perspective, that's great. From the developer perspective, I understands it's easy to put the sources of admesh to this repo and do whatever you want with them.

However, form the packager perspective, this is a nightmare, believe me.

Is there any chance you can use libadmesh instead? All of your changes, if they are not breaking anything, can be pushed to this repository as well (and other projects, like simarrange can benefit from them). I'm also open for Perl bindings pull requests.

Thanks very much for considering this approach.

@olasd
Copy link
Collaborator

olasd commented Nov 13, 2013

👍 from the prospective Debian packager !

I might be able to take a look at this this weekend.

@hroncok
Copy link
Member Author

hroncok commented Nov 19, 2013

@alexrj Do you think you can address this before the final 1.0.0 is released, or should I try it myself on Fedora level and possibly break something?

Please tell me:

  • Are your admesh commits backwards compatible and worth merging to admesh?
  • Would you prefer to create Perl bindings for admesh yourself or you don't have time for that and somebody else should do it?
  • If this effort is made, will you consider using such Perl bindings instead of bundling admesh?

Thanks.

@hroncok
Copy link
Member Author

hroncok commented Nov 20, 2013

Added 04d5d1b as hroncok/admesh@e524c2a (the tranlating part is in stl_translate_relative())

@hroncok
Copy link
Member Author

hroncok commented Nov 20, 2013

Added 1479d69 as hroncok/admesh@171d20c

@hroncok
Copy link
Member Author

hroncok commented Nov 20, 2013

Added 2a2633d as hroncok/admesh@9981d80

@hroncok
Copy link
Member Author

hroncok commented Nov 20, 2013

@alranel
Copy link
Member

alranel commented Nov 24, 2013

@hroncok, I don't know whether the changes are backwards compatible. They're needed for working with Slic3r.
When I took admesh code there was no maintainer nor your repository existed, so my only chance was to embed it. I don't know about the current compatibility.

To be honest, I'm reluctant on this. I don't like to depend on an external library which is poorly maintained (no offense) and lacks a testing suite to prevent regressions from people committing to its repository. The admesh code is so critical for Slic3r now, so I need to be able to fix bugs and change things on the fly should an issue arise. This is also why I removed the dependency on Boost.Geometry.

@hroncok
Copy link
Member Author

hroncok commented Nov 24, 2013

I see. Is there any chance you fork admesh, rename it to e.g. adm3sh and release it as a standalone library you maintain yourself (with your full control), or that's to much work for you? In that case that would no longer be bundling, but simple fork and that could work well.

Even if not, I' might be able to get an exception for bundling, but it's hard.

Thanks.

@alranel
Copy link
Member

alranel commented Nov 24, 2013

I'd say the compliancy with any distribution-specific packaging policy needs to be addressed at the packaging level rather than the upstream level. Why don't you just take a diff between my admesh and your distribution's admesh and check whether my patches are applicable? If they are, you can just link Slic3r to the library. If they aren't, you have a good reason for requesting an exception.

Also note that admesh is not a library. It's a command line utility. No API call is documented, and it wasn't designed to be a library. I took some code out of it and adapted it to Slic3r. So I think it's not entirely correct to consider it a library and apply the unbundling policies.

@alranel
Copy link
Member

alranel commented Nov 24, 2013

Don't get me wrong, I'm just overwhelmed by project management tasks and I have really no time to worry about packaging issues which I really think should be handled by the (patient!) packagers :(

@hroncok
Copy link
Member Author

hroncok commented Nov 24, 2013

No hard feeling, I just need to know what's your POV before I ask for exception.

So I guess I'll just try to continue applying your changes and when it will work I'll use it as a library and when not, I'll ask for exception. Thanks for your time and thanks for great work you've put into Slic3r :)

@alranel
Copy link
Member

alranel commented Dec 17, 2013

Thanks for understanding, @hroncok :)

@alranel alranel closed this as completed Dec 17, 2013
@hroncok
Copy link
Member Author

hroncok commented Jan 2, 2014

Is there a reason, why you change extern declaration to static?

@alranel
Copy link
Member

alranel commented Jan 2, 2014

I don't remember. What functions?

@hroncok
Copy link
Member Author

hroncok commented Jan 2, 2014

stl_read, stl_count_facets
While almost all functions in
https://github.com/alexrj/Slic3r/blob/master/xs/src/admesh/stl.h are extern, those two are static and it makes problems, when I wan to compile the library. Is there any particular reason, or extern is just fine?

@alranel
Copy link
Member

alranel commented Jan 2, 2014

I'd say extern is fine as well... I probably changed for clarity, as they are not supposed to be called from elsewhere.

alranel added a commit that referenced this issue Jan 2, 2014
Rename some admesh functions to preserve compatibility with oiriginal admesh #1525
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

No branches or pull requests

3 participants