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

expose the rust cargo.toml version string #422

Merged
merged 5 commits into from
May 19, 2022

Conversation

fundthmcalculus
Copy link
Contributor

No description provided.

native/src/ffi/utils.rs Outdated Show resolved Hide resolved
@tmarkovski
Copy link
Member

We didn't get a chance to discuss the approach before implementation. Here are my thoughts:

  • Getting version should be expanded to get additional metadata from native, not just version
  • The exposed method should use unversioned proto definition and FFI pattern. This is for consistency which applies to how data is managed and memory leaks prevented which is already implemented in all wrappers

@fundthmcalculus
Copy link
Contributor Author

We didn't get a chance to discuss the approach before implementation. Here are my thoughts:

  • Getting version should be expanded to get additional metadata from native, not just version
  • The exposed method should use unversioned proto definition and FFI pattern. This is for consistency which applies to how data is managed and memory leaks prevented which is already implemented in all wrappers

Agreed on all counts. I was thinking about this too.
void okapi_version(*ByteBuffer) with a proto definition like this:

message OkapiVersion {
    int major_version = 1;
    int minor_version = 2;
    int patch_version = 3;
    string pre_version = 4; // this should generally be empty, otherwise something like `-rc1`
    string git_commit_id = 5;
    // what others would you add?
}

@tmarkovski
Copy link
Member

tmarkovski commented May 18, 2022

The method definition can be the same as other FFI methods

int32_t okapi_metadata(struct ByteBuffer request,
                       struct ByteBuffer *response,
                       struct ExternError *err);
message MetadataRequest {
   repeated string variables = 1; // optional field, can contain any of the cargo env vars
}
message MetadataResponse {
  string version = 1;
  int version_major = 2;
  int version_minor = 3;
  // etc
  map<string, string> variables = 10; // will include any non default requested variables
}

@fundthmcalculus
Copy link
Contributor Author

That makes a lot of sense. Let's go with what you described. Is there a reason to return the full version in addition to the major and minor numbers? I think we should just return the version string and leave it at that. Definitely have the metadata map though.

@tmarkovski
Copy link
Member

Because it's simple to do so, and wrapper may make decisions based on different major versions, but not care about minor or patch changes for example.

@fundthmcalculus
Copy link
Contributor Author

Because it's simple to do so, and wrapper may make decisions based on different major versions, but not care about minor or patch changes for example.

That does make a good deal of sense. Since we aren't really releasing patch versions of the okapi binaries much, that should be enough.

@fundthmcalculus
Copy link
Contributor Author

These are only available at compile-time, so custom requesting of the information is not possible. env!() and option_env!() require a compile-time string constant, not a variable.

@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2022

Python Test Report

  12 files  ±0    12 suites  ±0   13s ⏱️ ±0s
  14 tests ±0    14 ✔️ ±0  0 💤 ±0  0 ±0 
168 runs  ±0  168 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 2971477. ± Comparison against base commit 7a35aef.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2022

Java Test Report

12 files  ±0  12 suites  ±0   2s ⏱️ ±0s
18 tests ±0  18 ✔️ ±0  0 💤 ±0  0 ±0 
42 runs  ±0  42 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 2971477. ± Comparison against base commit 7a35aef.

This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.
OkapiNativeTest ‑ [1] OkapiNativeTest$DataArgument@49ff7d8c
OkapiNativeTest ‑ [2] OkapiNativeTest$DataArgument@648c94da
OkapiNativeTest ‑ [1] OkapiNativeTest$DataArgument@524f3b3a
OkapiNativeTest ‑ [2] OkapiNativeTest$DataArgument@1b7c473a

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2022

Golang Test Report

  3 files  ±0    3 suites  ±0   1s ⏱️ ±0s
15 tests ±0  15 ✔️ ±0  0 💤 ±0  0 ±0 
45 runs  ±0  45 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 2971477. ± Comparison against base commit 7a35aef.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2022

Ruby Test Report

  48 files  ±0    48 suites  ±0   3s ⏱️ ±0s
  13 tests ±0    13 ✔️ ±0  0 💤 ±0  0 ±0 
156 runs  ±0  156 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 2971477. ± Comparison against base commit 7a35aef.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2022

.NET Test Report

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 2971477. ± Comparison against base commit 7a35aef.

♻️ This comment has been updated with latest results.

Copy link
Member

@tmarkovski tmarkovski left a comment

Choose a reason for hiding this comment

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

Can you do cargo fmt? It seems there's a file that needs formatting.

native/Cargo.toml Outdated Show resolved Hide resolved
@fundthmcalculus fundthmcalculus merged commit dd44bfa into main May 19, 2022
@fundthmcalculus fundthmcalculus deleted the okapi-421-expose-okapi-version branch May 19, 2022 11:43
@fundthmcalculus
Copy link
Contributor Author

I will handle rubocop and flake8 code style issues in a separate PR.

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

2 participants