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

Enable building E57Format as a shared library #40

Merged
merged 3 commits into from
May 29, 2020
Merged

Conversation

asmaloney
Copy link
Owner

No description provided.

@asmaloney
Copy link
Owner Author

@pscamodio could you please try this to see if it works for you?

The main differences from your PR are:

  • It doesn't set BUILD_SHARED_LIBS as an option, but it will respect it. (I don't like libraries polluting the cmake option-space.)
  • It uses GenerateExportHeader instead of hand-coding the include. This will generate the appropriate include based on the platform (__attribute__((visibility("hidden"))), etc.).

Copy link

@pscamodio pscamodio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi asmaloney
I'm testing this version of the lib and it seem to work.
But there's one problem with the cmake install
Currently if you build a shared library the .dll file is not installed
You need to add
RUNTIME DESTINATION bin to the install(TARGET call


install(
    TARGETS
        E57Format
    EXPORT
        E57Format-export
    ARCHIVE DESTINATION lib
+ ARCHIVE DESTINATION bin
)

@pscamodio
Copy link

pscamodio commented May 29, 2020

Thank you.
Everything works now.
Your library was the only reason for me to keep xerces around in my codebase.
Now I can package your library as a dll with xerces statically compiled inside and remove one public dependency from my tree.
Thank you

@asmaloney
Copy link
Owner Author

Thanks for prompting this change with your PR @pscamodio!

I would love to eliminate Xerces, but it's so embedded in this code it's not easy. I started to do it, but it really needs a rewrite.

@asmaloney asmaloney merged commit 88cfcd5 into master May 29, 2020
@asmaloney asmaloney deleted the dynamic-lib branch May 29, 2020 14:46
@Jean-Roc
Copy link

@asmaloney just found out this PR, you are mentioning getting ride of xerces, it seems the PDAL crew did it with their fork
https://github.com/PDAL/PDAL/commits/master/plugins/e57/libE57Format

@asmaloney
Copy link
Owner Author

@Jean-Roc Thanks. I didn't realize they forked my fork. I took a look at their history and I don't see where they replaced xerces.

Where did you see it - on a branch? What did they replace it with?

@Jean-Roc
Copy link

You are right, I made a mistake. I was trying to understand some differences in e57 handling between CC and pdal, found in their initial PR a mention of switching to libxml2, read the wrong cmakelist. Sorry for the noise

@asmaloney
Copy link
Owner Author

No problem. I wish they hadn't forked the fork though 😄 There are few enough people doing this that it would be nice to keep any fixes in one place so everyone can benefit. 🤷‍♂️

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