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

Updated from latest NI-DCPower exports #744

Merged
merged 27 commits into from
Oct 13, 2022

Conversation

reckenro
Copy link
Collaborator

@reckenro reckenro commented Sep 19, 2022

What does this Pull Request accomplish?

  • Updates NI-DCPower metadata/proto to come from hapigen export: nidcpower_grpc_device 23.0.0d114
  • Updates NI-DCPower header from hapigen export: nidcpower 23.0.0d114
  • Updates NI-DCPower nidcpower.lib from nidcpower 23.0.0d114

Breaking changes

  • Correction for wrong input on ErrorQuery: updated so ErrorQueryRequest doesn't have error_message field but instead the ErrorQueryResponse has it
  • ExportAttributeConfigurationBuffer and ImportAttributeConfigurationBuffer configuration fields updated from repeated fixed64 to bytes. This correction matches the header files and matches the similar function signatures in nidmm.proto and niscope.proto.

Why should this Pull Request be merged?

Covers the NI-DCPower part of the following task:

AB#2172471 : Import 23.0.x hapigen exports into grpc-device (nidcpower, niscope)

What testing has been done?

Built locally and NI-DCPower system tests still pass.

@dmondrik
Copy link
Contributor

@reckenro once your ni-central PR goes in, I'll pull together all the latest exports (dcpower and scope) and put them into an official PR for this.

@reckenro reckenro changed the title [Draft] Testing hapigen output for ni-dcpower Updated from latest NI-DCPower exports Oct 11, 2022
@reckenro reckenro requested a review from dmondrik October 11, 2022 16:23
@reckenro reckenro marked this pull request as ready for review October 11, 2022 16:24
@reckenro reckenro added the binary-breaking Change to proto file that requires client updates label Oct 11, 2022
@reckenro
Copy link
Collaborator Author

reckenro commented Oct 11, 2022

@astarche / @dmondrik , for the header and the metadata/proto I imported from 23.0.0d92 (latest export). But when doing the same for the nidcpower.lib file, I was getting linking errors for about six nidcpower_... methods.

Those methods line up with some recent changes that went into DC Power in hapigen which will be resolved with this AzDo bug. So I took the most recent export whose nidcpower.lib worked with the other changes in this PR. That happened to be nidcpower 23.0.0d83 which matches what I suspected because it is the export right before the problematic change went in (see ni-central commit)

All that being said, is it okay to include an older nidcpower.lib file or should we wait for that Bug to be fixed and get a new nidcpower.lib from a future export?

@dmondrik
Copy link
Contributor

All that being said, is it okay to include an older nidcpower.lib file or should we wait for that Bug to be fixed and get a new nidcpower.lib from a future export?

If an older import .lib file allows it to build correctly, then I think it's fine to proceed that way. There's no reason to block this arbitrarily.

generated/nidcpower/nidcpower.proto Outdated Show resolved Hide resolved
generated/nidcpower/nidcpower.proto Show resolved Hide resolved
@reckenro
Copy link
Collaborator Author

Windows system tests are currently busted because of issues with the grpcbot we run the tests on. It will be stuck in the queue forever. They pass locally and also run on Ubuntu as part of the run_ubuntu_system_tests step and passed there. So I'm going to cancel the run_win_system_tests step and submit.

@reckenro reckenro merged commit bf6e159 into ni:main Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binary-breaking Change to proto file that requires client updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants