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

[breaking] Fix export binaries binding not working in gRPC interface #1171

Merged
merged 1 commit into from Feb 11, 2021

Conversation

silvanocerza
Copy link
Contributor

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?

This fixes a export binaries confing binding not working in the gRPC interface.

  • What is the current behavior?

If a gRPC interface consumer sets sketch.always_export_binaries to true in its config file and sends a compilation request that that setting is ignored and binaries are not exported.

  • What is the new behavior?

gRPC interface consumer can now set sketch.always_export_binaries to true in their config file and sends a compilation request to correctly export binaries.

Yes, this PR contains a breaking change for gRPC consumers, the CLI is unaffected.

The gRPC interface CompileReq message had its ExportBinaries property type changed from bool to google.protobuf.BoolValue.

  • Other information:

Solves #1103.


See how to contribute

@kittaakos
Copy link
Contributor

kittaakos commented Feb 11, 2021

The CLI correctly picks up sketch.always_export_binaries from the CLI config so that part is OK. Unfortunately, it breaks Export compiled Binaries. I cannot force to export the binary if sketch.always_export_binaries is false in the config.

Update:
So the IDE was to use the default behavior from the CLI config, but when it is forced to export, it does not work:

if (typeof options.exportBinaries === 'boolean') {
    const exportBinaries = new BoolValue();
    exportBinaries.setValue(options.exportBinaries);
    compilerReq.setExportBinaries(exportBinaries);
}

This ☝️ has no effect.

@silvanocerza
Copy link
Contributor Author

That's unfortunate, I'll take a look at it.

@kittaakos
Copy link
Contributor

Now I am puzzled. Let me do a couple of additional experiments, as it works now. It wasn't before.

@kittaakos
Copy link
Contributor

Let me do a couple of additional experiments, as it works now. It wasn't before.

I can confirm, it works as expected. There was a build issue on my side. Thank you for the fix, @silvanocerza 👍, and sorry for the confusion.

@silvanocerza silvanocerza force-pushed the scerza/fix-export-binaries-binding branch from 8026881 to 5d69fdc Compare February 11, 2021 13:45
@silvanocerza
Copy link
Contributor Author

Rebased to include integration tests fixes from #1183

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