-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Static build #813
base: master
Are you sure you want to change the base?
Static build #813
Conversation
Extracted code for static build from Automattic#571 to make it easier to review and approve. It's a squashed version with the final code, for references on the development history and commit logs go to the original issue.
Transferring from original PR: This doesn't build on Windows because there are shell scripts in binding.gyp, static.gyp and canvas.gypi. There's discussion in #754 about replacing has_lib.sh with something sturdier: either letting the compiler find the libs or porting it to JS. |
Well, this doesn't build on Windows, but node-canvas doesn't do it too. I El 19/9/2016 1:51, "Zach Bjornson" [email protected] escribió:
|
Not sure I understand your reply. node-canvas builds on Windows currently. |
Then what's the problem with bash? The 'has_lib.sh' script was already El 19/9/2016 8:09, "Zach Bjornson" [email protected] escribió:
|
has_lib.sh isn't used on Windows. Look at the top of the existing binding.gyp file; there's a check for Windows. |
Ok, now I understand it. Fine, I have three options: state that static El 19/9/2016 8:19, "Zach Bjornson" [email protected] escribió:
|
Supporting Windows somehow would be nice, but better to get the advice of the official maintainers... |
I agree. El 19/9/2016 8:25, "Zach Bjornson" [email protected] escribió:
|
It's kind of unfortunate that it has to have so many My solution here was to build normally and then copy the shared libraries into the Release folder by looking at what |
This is not so much complex, it's only that Cairo has a lot of El 19/9/2016 15:20, "Caleb Hearon" [email protected] escribió:
|
Regarding the dynamic libraries, I proposed to Node.js dev something El 19/9/2016 15:41, escribió: This is not so much complex, it's only that Cairo has a lot of El 19/9/2016 15:20, "Caleb Hearon" [email protected] escribió: It's kind of unfortunate that it has to have so many gyp files and custom My solution here https://github.com/chearon/node-canvas-prebuilt was to — |
Please consider this for 2.0. |
Any update on this one? |
Could this be reviewed by any of the project maintainers? It has started to get some conflicts with master but didn't get yet any news about its mergeability status... :-/ |
I'm sorry for the delay on this, it's a rather large change and I haven't got much time. I'll try to get some open source work done this weekend so hopefully I can look thru this... |
Cool :-) If you need some help don't doubt to ask :-) |
Would love to see prebuilds for node-canvas. ❤️ |
Code rebased with changes on master. |
@piranna Do I understand this PR right as in it generates a statically linked version of |
Yes, that's it, there's no runtime dependencies. It checks on build time if Cairo library is available to do a dynamic build as usual, and if not, it does a static one. Static build can be forced too by passing the |
FWIW, this constantly fails to build on my Mac, exits with the following log:
|
static/ensure_deps.sh
Outdated
LIBJPEG_URL=http://downloads.sourceforge.net/project/libjpeg-turbo/$LIBJPEG_VERSION/libjpeg-turbo-$LIBJPEG_VERSION.tar.gz | ||
LIBPNG_URL=http://sourceforge.net/projects/libpng/files/libpng12/$LIBPNG_VERSION/libpng-$LIBPNG_VERSION.tar.xz | ||
PIXMAN_URL=http://cairographics.org/releases/pixman-$PIXMAN_VERSION.tar.gz | ||
ZLIB_URL=http://zlib.net/zlib-$ZLIB_VERSION.tar.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link is no longer valid as zlib has updated to 1.2.11.
For sanity, this should be pointing to sourceforge as well, as zlib doesn't seem to keep history of their old releases.
https://sourceforge.net/projects/libpng/files/zlib/1.2.8/zlib-1.2.8.tar.gz works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just working on that, I'll push a commit with the fixes when it's ready, sorry for the inconvenience.
Does everyone really think it's a good idea to be tracking 3rd-party header files and hard-coded links to external tarballs? And a custom gyp file for each dependency? This is really complicating an already complicated build. The approach @eonarheim outlined in #641 doesn't change the build. If this does get merged, there's also the other half of the problem, how does @LinusU publish the binaries to NPM? |
prebuild-install is your friend :-) |
OK, but it still has to be done on each of the 3 platforms. So assuming we do that through CI servers, someone has to set up each of those |
I have just send an email to @bnoordhuis asking for info about how we can be able to add those gyp files in the gypified organization. |
# Conflicts: # .gitignore # .npmignore # binding.gyp # util/has_lib.js
Always amazing to see this PR continuing on. 😄 |
What do you mean? |
I just happened to notice there were more commits and wasn't merged. Sorry for the notification spam. |
No problem, just wanted to keep my pull-requests updated to make them easier to be merged someday :-) Or at least get some feedback about what does they need to be improved. |
I've seen by chance what was the issue here and fixed it, and now tests are passing on Windows, Linux and macOS. Windows has disabled support for static builds, probably it could be possible too but I don't use Windows so I can't try. |
Code sync'ed with latest |
@piranna, that's some serious gyp! In case others like myself are seeking to quarantine node-gyp nuances in favor of CC flavored cli and make/cmake/etc builds, here's a PoC node-gyp wrapper, node-gypcc, that conforms to gcc/clang/emcc cli. It's early (released <24hrs ago, PRs welcome). Hope it helps, cheers! |
Did anything come off this? I'm still very hesitant to adding the vast amount of header files that this patch currently brings 😅 |
What issues do you see? |
It brings in ~6000 new lines of code that we need to maintain. I'm not familiar with the process enough, which is really the biggest problem. e.g. Another example is that it hardcodes Will this work with Apple aarch64 platform? Or arm64 Linux? Or is this something that we now need to add support for manually? 🤔 |
I just generated the config files with the default options for all the needed libraries, similar to how it was done in other ones. Must to admit I'm not fully happy with having so much hardcoded options too, but seems it's the only way. Maybe they can be generated on build time too, but I'm not sure.
Honestly, no idea... :-( |
In any case, some of my other PRs regarding screen drawring are more useful and easier to merge, maybe we can review them first and later retake this one, in case it's still needed, what do you think? |
I'm talking about https://github.com/Automattic/node-canvas/pulls?q=is%3Aopen+is%3Apr+author%3A%40me+sort%3Aupdated-desc, but more specifically, #1543. |
In any case, maybe this PR can be splitted in two, one for the gyp config files to allow static vs. dynamic libraries, and another one with the specific config changes, in case it's still needed. |
Despite all the hard work, unfortunately I don't really see a good path forward still.
I don't really have suggestions either unfortunately. It would be worth looking at how Firefox deals with this scenario since they use a lot of the same dependencies. |
64ed3d8
to
ff0f2ab
Compare
Extracted code for static build from #571 to make it easier to review and
approve. It's a squashed version with the final code, for references on the
development history and commit logs go to the original issue.