Skip to content
This repository has been archived by the owner on Jan 31, 2023. It is now read-only.

Add grpc prebuilt binaries somewhere #84

Closed
cazfletch opened this issue Sep 13, 2018 · 7 comments
Closed

Add grpc prebuilt binaries somewhere #84

cazfletch opened this issue Sep 13, 2018 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@cazfletch
Copy link
Contributor

Rebuilding the grpc binaries takes ages we should try and add them somewhere

@cazfletch cazfletch added the bug Something isn't working label Sep 13, 2018
@cazfletch cazfletch added this to the VSCode Extension phase 1 milestone Sep 13, 2018
@jt-nti
Copy link
Contributor

jt-nti commented Sep 14, 2018

Just a few notes after some initial investigation...

The recommended approach appears to be,

to include binaries for all four platforms of VS Code (Windows x86 and x64, Linux, macOS) in your extension and have code that dynamically loads the right one.

The good news is that it looks like node-pre-gyp should handle this as long as all the binaries are there when creating the extension.

The less good news is that there is currently an issue selecting the correct binary in forks - mapbox/node-pre-gyp#278

@cazfletch
Copy link
Contributor Author

While that is the recommend approach it makes the extension size really big. We shouldn’t include all the binaries

@jt-nti
Copy link
Contributor

jt-nti commented Sep 14, 2018

It should add around 12M to the extension size, which isn't nothing but doesn't seem like the end of the world. What's the requirement for keeping the extension size down? Is there a target size? Target install time? (I would personally rather that the extension had actually finished installing once it's installed, which is not the case if you defer additional downloads or end up doing builds.)

@cazfletch
Copy link
Contributor Author

The microsoft c++ extension does the downloading after rather than including it. Which goes against what they recommend in their docs.

I'd worry that we wouldn't stop at 4 versions. For example in the future with new versions of vscode if we chose to say that the minimum level we require is not the latest then we would need to include binaries for all the levels of different electron versions.

We don't have a target size or time but trying to keep it as small as possible to make downloads of it quicker. You would end up with 3 binaries ( or possibly more) that you didn't need.

@jt-nti
Copy link
Contributor

jt-nti commented Sep 17, 2018

As discussed with @caroline-church, it seems like the options are...

  1. Bundle binaries directly

    • Pros: reliable, no post-install surprises/builds, under our control
    • Cons: bigger extension, includes unnecessary binaries
  2. Download binaries post-install

    • Pros: under our control, smaller extension, only download required binary for platform, no more build!
    • Cons: post-install surprise (eg. no network on activation), we'd have to build/host the binaries somewhere and make sure that's reliable
  3. Attempt to add platforms to grpc pre-built binaries

    • Pros: smaller extension, only download required binary for platform, build fallback if they're not available for any reason, we don't have to host anything
    • Cons: post-install surprise (e.g. a build), not under our control, may need to repeat the exercise if vscode moves to later electron version but grpc don't publish pre-built binaries for the new combos in a timely manner

Since option 1 is probably the quickest way to get to a point that grpc won't need building (takes much longer than a download), and it can still fall back to building if required (e.g. if vscode change the electron version without a major version bump), that's our current preferred option.

@jt-nti
Copy link
Contributor

jt-nti commented Sep 21, 2018

While investigating how to build all the binaries into a single extension, it turned out that grpc already has prebuilt binaries for electron 2.0, just not for the old versions specified by the fabric node sdk client. I've opened an issue for updating the version of grpc which should make life a bit easier in the future:

https://jira.hyperledger.org/browse/FABN-930

@jt-nti
Copy link
Contributor

jt-nti commented Sep 21, 2018

Fixed in hyperledger/fabric-sdk-node@6366fac

@jt-nti jt-nti closed this as completed Sep 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants