-
-
Notifications
You must be signed in to change notification settings - Fork 10
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 version information to API #61
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for PR! A few minor comments
roc/version.go
Outdated
Patch: uint64(cVersion.patch), | ||
} | ||
|
||
if info, ok := debug.ReadBuildInfo(); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading your comment #61 (comment) I realized that debug.ReadBuildInfo() likely will return the version of main go module, not the version of module from which it is called.
i.e. if user program foo uses roc, ReadBuildInfo() will return version of foo, not version of roc.
Could you please check if this is true? If yes, then ReadBuildInfo is not suitable for us :/
I see the following alternative solution:
- hard-code bindings version in source code (like,
const bindingsVersion = "1.2.3"
) - add CI check that runs only for tag (i.e.
v*
) that verifies, that version in source code and version in git tag are identical
We'll need to update hard-coded version in source code each release. If we forget to do so, CI will notify us.
We use similar approach in roc-droid:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of notify us via CI, maybe can try push the newer version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems need to give permission to github actions first before it can push some changes
so updated to check only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of notify us via CI, maybe can try push the newer version
What do you mean by pushing new version?
BTW welcome to matrix chat! https://roc-streaming.org/toolkit/docs/about_project/contacts.html |
81e8f36
to
44c20ba
Compare
Amazing job! It would make it even better if you included some motivation / |
roc/version_test.go
Outdated
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestVersion(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great test, although it does only test a successful path. I can see that the way Version
func is implemented you cannot really inject any test strings into it and test failures here. Have you thought about decoupling it to make it possible? There are a lot of moving parts in Version
and it can crap out in e. g. lines 40, 47, 51, 52 and 53. I am not sure if it is vital, but I would suggest it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you thought about decoupling it to make it possible?
yes already thought about this, but seems ok to me to ignore and try to return as much as possible instead of returning error
There are a lot of moving parts in Version and it can crap out in e. g. lines 40, 47, 51, 52 and 53. I am not sure if it is vital, but I would suggest it.
yes those lines can fail when parsing invalid bindingsVersion
let me try add 1 unit test to check whether the bindingsVersion
is valid or not so we can check it early in unit test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes already thought about this, but seems ok to me to ignore and try to return as much as possible instead of returning error
In general, I would not think it's ok to do it, but I can see your motivation, because Version
returns {0,0,0} in case of error anyway. Let's see how that discussion plays out, so depending on what we decide, you would leave this func as is or rewrite it a bit to decouple getting the version from C and parsing it to allow for more modularity and testability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me try add 1 unit test to check whether the bindingsVersion is valid or not so we can check it early in unit test
I like this idea. User interface is simpler (no need to check for errors from roc.Version), and at the same time our test suite guarantees that failures are not possible (by validating hard-coded version).
The only downside is that the test duplicates parsing logic. If we update version.go and forget version_test.go, then the test's validation may become irrelevant.
So I'd suggest to refactor it to reuse parsing code. One way to do it:
- extract private function parseVersion that accepts string, returns version triple, or panics on invalid string format
- in roc.Version, use parseVersion
- in tests, check that:
- parseVersion works on valid input
- parseVersion panics on invalid input
- parseVersion works in hard-coded bindingsVersion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from me, @Asalle feel free to resolve this
roc/version_test.go
Outdated
} | ||
|
||
func Test_bindingsVersion(t *testing.T) { | ||
bvs := strings.SplitN(bindingsVersion, ".", 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! Does it pass locally for you? I'm not sure where does the var bindingsVersion
come from, and on the CI it this test seems to be skipped. I haven't run it locally, let me try it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it pass locally for you?
yes
I'm not sure where does the var bindingsVersion come from, and on the CI it this test seems to be skipped
it's from latest release, it's skipped because need to push changes with tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Asalle see also this #61 (comment)
I originally suggested to use ReadBuildInfo, then we realized that it's not suitable for libs, only for main go module; thus I suggested to hard-code version and in CI step for git tag check that the hard-coded version matches the latest git tag
It means that we'll have to update hard-coded version each release, manually (and CI for release will fail if we forget)
We came to this approach in roc-droid and openfec. I don't like it too much, but I just don't have better ideas. Maybe you have some?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember injecting an env var during build like so, with ldflags, didn't try it with libs though, should work with go build, right? Worth a try! Seems like you can inject that stuff into packages as well, not just main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion moved to chat
Added 3 minor comments, otherwise looks fine. Waiting for review from Asa. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, amazing job and thanks for your contribution!
close #47