Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement basic server and test probe #12
Implement basic server and test probe #12
Changes from 2 commits
f3d6747
f8c1be7
aca35c1
08e701d
d6aced0
61e0ff8
da701b8
a44c1ec
80d2029
d261261
e9b00bc
5a37564
25fbe2f
e3fcf83
4b30780
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Somewhat philosophical: cargo follows its own flavour of semver rules, so specifying
bincode = "1.3.3"
is equivalent to^1.3.3
, which pulls1.3.x
where x>= 3
. So if we don't specifically need the patch version to be at least 3, we can as well sayThat gives
cargo
dependency resolver a bit more leeway to deduplicate dependencies in some corner cases (think of some transitive dependency depending onbincode = "=1.3.2"
or similar.The downside is that it is more work to do this "properly" - if
bincode 1.3.1
introdice a (backwards-compatible) feature that we use, we shoudl depend on1.3.1
and not1.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.
In tonari codebase, we typically do:
Only specify the major version past 1.0, and specify minor version before 1.0. This will give cargo the most flexibility assuming that our dependent crates follow the semvar compatibility requirements strictly. @strohel should we do that here as well?
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.
@skywhale yeah that's basically what I intended to say (it convoluted way) in my above comment.
The only exception/caveat is that patch (x.y.Z+1) releases can introduce new features (if they're backwards compatible), and we can depend on the new features. So sometimes specifying all 3 components for dependencies is needed/correct.
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 was lazy here and just rolled with whatever
cargo add ...
produced.Trying to not block this PR further, in 80d2029 I changed the versions to what @skywhale suggested.
I'll try to get to the philosophical aspect later. Now, I need to go out with the dog. .)
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.
Getting back to the caveat @strohel mentions: I think that there is little reason to aim for the correct (i.e. minimal functional lower bound) version range from the get go. We'd have to essentially enumerate all the "features" we use and backtrack them to the minor version they were all included in. That'd be hard.
Also remember, that there's cargo.lock. If someone adds a
dependency = "1.5"
and makes our program work but in fact relies on a feature from1.5.5
then the exact version pinned in cargo.lock must be1.5.5+
. I.e. the caveat can only arise when cargo.lock changes which isn't as often and it should be fairly easy to spot what happened in that case. At least as long as the version mismatch manifests visibly - which is something I'd expect in a language typed as strongly as rust.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.
@goodhoko agreed. Perhaps libraries (which do not ship
Cargo.lock
) that are popular should do this, but indeed that seems like an overkill for us.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'm still unsure about using
Udp
instead ofTcp
for this.If we were talking about sending audio packets or any other kind of short, time-limited, missable data I would definitely use Udp. However, if I understand where we are in terms of abstraction when it comes to events, an event could signify a single occurrence of something very bad. For example, a single event packet could mean reporting a catastrophic error that would result in a few seconds of screeching (I may be wrong on this though since I don't have the context of your in-person chats so correct me if I am!).
If I'm on the right ballpark about the potential importance of events though, we shouldn't be missing any, and I think the reliability of TCP would work better here.
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.
Right. As agreed personally, we'll continue here with UDP but should look into using reliable alternatives. I filled an issue for that #15
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.
Minor nit: the
continue
isn't needed anymore.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.
removed in 4b30780
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.
Nit: not super urgent, but I like using
clap
for all the argument handling so we get all the bells and whistles like usage strings, --help menu, etc :)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's do this in a sperate PR. I filled an issue for myself #16