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 bundling of 3rdparty apps for receipt-scanner #7

Merged
merged 16 commits into from
Oct 17, 2016

Conversation

danschultzer
Copy link
Owner

@danschultzer danschultzer commented Sep 28, 2016

Resolves #2

  • Remove unnecessary files
    • Remove all share/man files
    • Remove all include files
  • Set PKG_CONFIG_PATH env var on npm install (added readme instead)
  • Only build on Mac OS X systems (don't build on Circle CI)
  • Use Core Graphics instead of compiling libjpeg, libtiff, etc? (can't be used)
  • Test on sandboxed/clean system to see that the binaries work as expected
  • Test JPEG2000 files

@danschultzer danschultzer force-pushed the build-and-bundle-3rdparty-dependencies branch 5 times, most recently from 5ff7714 to 1982759 Compare September 29, 2016 05:19
@danschultzer
Copy link
Owner Author

Release builds now work, but with one caveat. To build it, you need to define PKG_CONFIG_PATH. Run:

PKG_CONFIG_PATH=$(pwd)/thirdparty/opencv/lib/pkgconfig npm run release

This should be included somewhere in the dev setup though so it happens automatically.

Current file size is 389.6 MB. Breakdown:

  • app.asar: 78.5 MB
  • electron: 116.9 MB
  • thirdparty: 194.7 MB
    • thirdparty/dependencies: 29.8 MB
    • thirdparty/opencv: 53.7 MB
    • thirdparty/opencv: 14.8 MB
    • thirdparty/tesseract: 96.4 MB

@danschultzer danschultzer force-pushed the build-and-bundle-3rdparty-dependencies branch from e067fe2 to 5df3266 Compare October 1, 2016 05:29
@codecov-io
Copy link

codecov-io commented Oct 1, 2016

Current coverage is 93.37% (diff: 100%)

Merging #7 into master will not change coverage

@@             master         #7   diff @@
==========================================
  Files             3          3          
  Lines           166        166          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            155        155          
  Misses           11         11          
  Partials          0          0          

Powered by Codecov. Last update 3ddd9e8...f18e930

@danschultzer
Copy link
Owner Author

Overall this PR is ready for 👀, and can be shipped. There're a few minor things that can be done, namely removing all unnecessary files. We can probably remove 20MB+.

@danschultzer
Copy link
Owner Author

Current file size is 332.8 MB. Breakdown:

  • app.asar: 49.6 MB
  • thirdparty: 174.8 MB
    • thirdparty/dependencies: 20.7 MB
    • thirdparty/opencv: 43.6 MB
    • thirdparty/opencv: 14.8 MB
    • thirdparty/tesseract: 95.6 MB

DMG: 103.5 MB
Zip: 109.9 MB

I think this is good for now. @Schultzer can you take a 👀 on this and let me know if there's anything else we could rip out of third party dependencies.

@danschultzer danschultzer force-pushed the build-and-bundle-3rdparty-dependencies branch 8 times, most recently from cf1cdb6 to 9bdadb5 Compare October 7, 2016 22:31
@danschultzer
Copy link
Owner Author

Removed a lot of unnecessary opencv libraries.

Current file size is 302.6 MB. Breakdown:

  • app.asar: 49.6 MB
  • thirdparty: 143.1 MB
    • thirdparty/dependencies: 20.7 MB
    • thirdparty/opencv: 12 MB
    • thirdparty/poppler: 14.8 MB
    • thirdparty/tesseract: 95.6 MB

DMG: 94.2 MB
Zip: 99.1 MB

@danschultzer danschultzer force-pushed the build-and-bundle-3rdparty-dependencies branch 2 times, most recently from 849b487 to 220c81a Compare October 7, 2016 23:06
@danschultzer
Copy link
Owner Author

I've added all eng related tessdata files now since we excluded quite a bit by just using eng.traineddata. Also made Travis cache much smaller by not caching tessdata-master.

@danschultzer danschultzer force-pushed the build-and-bundle-3rdparty-dependencies branch 2 times, most recently from 79a3c8a to b2992b7 Compare October 8, 2016 00:35
@danschultzer
Copy link
Owner Author

danschultzer commented Oct 8, 2016

Removed a lot of unnecessary opencv modules, but this causes issues upstream with node-opencv. I'll see if I can make a temporary fix that we can use.

In file included from ../src/init.cc:1:
In file included from ../src/OpenCV.h:19:
/Users/dan/blazing-bookkeeper/thirdparty/opencv/include/opencv/cv.h:66:10: fatal error: 'opencv2/video/tracking_c.h' file not found
#include "opencv2/video/tracking_c.h"

@danschultzer danschultzer force-pushed the build-and-bundle-3rdparty-dependencies branch from b2992b7 to 3ef00ed Compare October 9, 2016 02:43
@danschultzer
Copy link
Owner Author

Was a lot of work to get node-opencv to work with available modules, but it's working now. I'll put up a PR soon to fix it upstream, but for now, we'll just use the branch to build.

@danschultzer danschultzer force-pushed the build-and-bundle-3rdparty-dependencies branch 3 times, most recently from e55208b to 17bd239 Compare October 9, 2016 03:34
@danschultzer danschultzer force-pushed the build-and-bundle-3rdparty-dependencies branch from 17bd239 to 677f77a Compare October 9, 2016 06:21
@danschultzer danschultzer force-pushed the build-and-bundle-3rdparty-dependencies branch 3 times, most recently from 90577b5 to 2d41235 Compare October 16, 2016 03:21
@danschultzer
Copy link
Owner Author

The last detail in this is libopenjpeg and jasper. Poppler throws this warning when we compile it without libopenjpeg:

Warning: Using libopenjpeg is recommended. The internal JPX decoder is unmaintained.

I'm not sure if we should support JPEG 2000 (.jp2 and .jpx) or not. I would vote for disabling support completely, but we can do a quick test with PDF containing jpeg 2K images to see how it'll work or break.

The support is limited: http://caniuse.com/#feat=jpeg2000

@danschultzer
Copy link
Owner Author

Removed libopenjpeg completely. Ready for final review, and this can be merged in.

@danschultzer danschultzer force-pushed the build-and-bundle-3rdparty-dependencies branch from 31c80bb to dee4000 Compare October 16, 2016 22:24
We only need to run the build scripts on mac when doing release.
- Moved thirdparty dependencies to Resources location
- Removed most tesseract trained data, just keeping eng and osd (orientation)
- Update readme with PKG_CONFIG_PATH instructions
There’s not really a need for libopenjpeg, since only poppler required it, and it was not compiled into anything else. Enabled zlib again on leptonica.
@danschultzer danschultzer force-pushed the build-and-bundle-3rdparty-dependencies branch from dee4000 to 9137052 Compare October 17, 2016 01:26
Copy link
Sponsor Collaborator

@Schultzer Schultzer left a comment

Choose a reason for hiding this comment

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

We should follow #21 with a uniform syntax and use single quotation marks and only use double quotation marks when escaping single quotation in out JS code base.

var thirdpartyPath = getResourcesPath('thirdparty'),
env = {};

if (process.platform == 'darwin') {
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

(process.platform == 'darwin') this should be (process.platform === 'darwin')

-D WITH_IPP=ON \
-D WITH_OPENCL=OFF -D WITH_CUDA=OFF -D BUILD_opencv_gpu=OFF -D BUILD_opencv_gpuarithm=OFF -D BUILD_opencv_gpubgsegm=OFF -D BUILD_opencv_gpucodec=OFF -D BUILD_opencv_gpufeatures2d=OFF -D BUILD_opencv_gpufilters=OFF -D BUILD_opencv_gpuimgproc=OFF -D BUILD_opencv_gpulegacy=OFF -D BUILD_opencv_gpuoptflow=OFF -D BUILD_opencv_gpustereo=OFF -D BUILD_opencv_gpuwarping=OFF -D BUILD_opencv_shape=OFF -D BUILD_opencv_stiching=OFF -D BUILD_opencv_superres=OFF -D BUILD_opencv_objdetect=OFF \
-D BUILD_TIFF=OFF -D BUILD_PNG=OFF -D BUILD_JPEG=OFF -D BUILD_ZLIB=OFF -D WITH_JASPER=OFF -D WITH_OPENEXR=OFF \
-D BUILD_opencv_highgui=OFF -D BUILD_opencv_flann=OFF -D BUILD_opencv_ml=OFF -D BUILD_opencv_calib3d=OFF -D BUILD_opencv_videoio=OFF -D BUILD_opencv_video=OFF -D BUILD_opencv_videostab=OFF -D BUILD_opencv_java=OFF -DBUILD_opencv_python=OFF \
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

-DBUILD_opencv_python=OFF This should be -D BUILD_opencv_python=OFF

if (process.platform == 'darwin') {
// Set 3rd party binaries and libraries
env.PATH = [
"$PATH",
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

"$PATH" should be '$PATH' we should use single quotation marks for strings

// Set 3rd party binaries and libraries
env.PATH = [
"$PATH",
thirdpartyPath + "/poppler/bin",
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

"/poppler/bin" should be '/poppler/bin'

env.PATH = [
"$PATH",
thirdpartyPath + "/poppler/bin",
thirdpartyPath + "/tesseract/bin"
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

"/tesseract/bin" should be /tesseract/bin

gutil.log("Going to build for", gutil.colors.cyan("'" + platform + "'"));

var buildCommand = [
"BUILDDIR=" + buildDir,
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

"BUILDDIR=" should be 'BUILDDIR='


var buildCommand = [
"BUILDDIR=" + buildDir,
"THIRDPARTYDIR=" + buildDestDir,
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

"THIRDPARTYDIR=" should be 'THIRDPARTYDIR='

"BUILDDIR=" + buildDir,
"THIRDPARTYDIR=" + buildDestDir,
'sh ' + buildScript,
].join(" ");
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

join(" "); should be join(' ');

},
function(error, stdout, stderr) {
if (!error) {
gutil.log("Build succesful");
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

gutil.log("Build succesful"); should be gutil.log('Build succesful');

gutil.log("Build succesful");
resolve();
} else {
gutil.log("Build failed");
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

gutil.log("Build failed"); should be gutil.log('Build failed');

@danschultzer danschultzer force-pushed the build-and-bundle-3rdparty-dependencies branch from 08c07bc to f18e930 Compare October 17, 2016 04:12
@danschultzer danschultzer merged commit cd42ccb into master Oct 17, 2016
@danschultzer danschultzer deleted the build-and-bundle-3rdparty-dependencies branch October 17, 2016 04:28
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.

3 participants