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

Add CMake build script to the project #209

Merged
merged 3 commits into from
May 9, 2022

Conversation

tamasmeszaros
Copy link
Contributor

@tamasmeszaros tamasmeszaros commented May 4, 2022

Hello there! I'm sending this PR on behalf of the PrusaSlicer team. We are using nanosvg in our software and having trouble with symbol duplication since wxWidgets also started using this library.

Since both our team and the wxWidgets team is using CMake, I've written the CMake build and install scripts to be able to import the library in both projects without duplication issues.

I've also added a short extension to the readme file to address the new integration method.

I know that this goes somewhat against the philosophy of the library to be simple to integrate and the examples already use a different build system but this is the solution we could provide quickly and believe it would have the most widespread acceptance in the community.

@tamasmeszaros
Copy link
Contributor Author

Related discussion wxWidgets/wxWidgets#22393

@memononen
Copy link
Owner

@tamasmeszaros I'm willing to merge this if I can summon you to fix the cmake issues in the future :) Would that work for you?

@tamasmeszaros
Copy link
Contributor Author

Hi @memononen! Thank you, I can handle it for you.

@tamasmeszaros
Copy link
Contributor Author

tamasmeszaros commented May 9, 2022

It would be good to consider the versioning before merging the PR. What concerns me is this line b21ebe0#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR25

Which assumes that this library is at version 1.0 and any matching or newer version is usable as a drop-in. This versioning thing could be omitted but I think it's safer to leave it there and agree on a version now, and maybe even make the compatibility somewhat stricter, (e.g. only matching major versions are compatible). But if you are confident that the library is now finished and will not change much in the future, it's not that big of a deal.

@memononen
Copy link
Owner

Good point about versions. With the time i have to maintain this project (which is close to zero), it's quite hard to maintain releases. I'm fine with 1.0, and if that allows us to later differentiate incompatible versions, that is great.

@bubnikv
Copy link

bubnikv commented May 11, 2022

Great, thanks to all involved!

@@ -25,6 +25,8 @@
#ifndef NANOSVGRAST_H
#define NANOSVGRAST_H

#include "nanosvg.h"
Copy link

@akien-mga akien-mga May 17, 2022

Choose a reason for hiding this comment

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

With this change, it's no longer possible to include nanosvgrast.h after including nanosvg.h as it leads to duplicate symbols:

$ cat << EOF > nanosvg.cc
#include "stdio.h"
#include "string.h"
#include "math.h"
#define NANOSVG_ALL_COLOR_KEYWORDS
#define NANOSVG_IMPLEMENTATION
#include "nanosvg.h"
#define NANOSVGRAST_IMPLEMENTATION
#include "nanosvgrast.h"
EOF

$ g++ nanosvg.cc
In file included from nanosvgrast.h:28,
                 from nanosvg.cc:8:
nanosvg.h:219:12: error: redefinition of ‘int nsvg__isspace(char)’
  219 | static int nsvg__isspace(char c)
      |            ^~~~~~~~~~~~~
In file included from nanosvg.cc:6:
nanosvg.h:219:12: note: ‘int nsvg__isspace(char)’ previously defined here
  219 | static int nsvg__isspace(char c)
      |            ^~~~~~~~~~~~~
nanosvg.h:224:12: error: redefinition of ‘int nsvg__isdigit(char)’
  224 | static int nsvg__isdigit(char c)
      |            ^~~~~~~~~~~~~
nanosvg.h:224:12: note: ‘int nsvg__isdigit(char)’ previously defined here
  224 | static int nsvg__isdigit(char c)
      |            ^~~~~~~~~~~~~
nanosvg.h:229:26: error: redefinition of ‘float nsvg__minf(float, float)’
  229 | static NSVG_INLINE float nsvg__minf(float a, float b) { return a < b ? a : b; }
      |                          ^~~~~~~~~~
nanosvg.h:229:26: note: ‘float nsvg__minf(float, float)’ previously defined here
  229 | static NSVG_INLINE float nsvg__minf(float a, float b) { return a < b ? a : b; }
      |                          ^~~~~~~~~~
nanosvg.h:230:26: error: redefinition of ‘float nsvg__maxf(float, float)’
  230 | static NSVG_INLINE float nsvg__maxf(float a, float b) { return a > b ? a : b; }
      |                          ^~~~~~~~~~
nanosvg.h:230:26: note: ‘float nsvg__maxf(float, float)’ previously defined here
  230 | static NSVG_INLINE float nsvg__maxf(float a, float b) { return a > b ? a : b; }
      |                          ^~~~~~~~~~
...

and dozens more errors.

Copy link
Owner

Choose a reason for hiding this comment

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

That include should not be there, my bad reviewing it. @tamasmeszaros would you mind making another PR fixing that?

Copy link
Contributor Author

@tamasmeszaros tamasmeszaros May 26, 2022

Choose a reason for hiding this comment

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

Yes, but I would not remove the include but rather extend the include guards for the whole files for nanosvg.h and nanosvgrast.h.

See #215
I've also added a bonus PR #216 with a CMake build script for the examples. Use it if you like it :)

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.

6 participants