-
Notifications
You must be signed in to change notification settings - Fork 0
publish: add source code and CI. #1
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
Conversation
| @@ -0,0 +1,180 @@ | |||
| /* | |||
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 would rather split it in dlt-rs-sys and dlt-rs with safe rust api. At least this is the best practice
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 would prefer dlt-rs and dlt-sys. As the dlt-rs doesn't interact with the sys library anymore but only the rust wrapper.
Would be similar to openssl and in line with what you suggested in slack.
dlt-tracing-appender/Cargo.toml
Outdated
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| [package] | ||
| name = "dlt-tracing-appender" |
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 would favor <vendor>-tracing-dlt. This follows the naming schema in the tracing repository.
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.
would 'tracing-dlt' be okay for you?
i.e. otel doesn't have a vendor either
https://docs.rs/tracing-opentelemetry/latest/tracing_opentelemetry/
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.
Totally fine with me. I thought that if we push this to crates.io we might cause conflicts with the tracing- prefix. Feel free to use tracing-dlt
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.
done, dlt-tracing is still free on crates.io :)
(Side note, we're checking if an how we can also publish this on crates.io)
Signed-off-by: Alexander Mohr <alexander.m.mohr@mercedes-benz.com
Signed-off-by: Alexander Mohr <alexander.m.mohr@mercedes-benz.com
Signed-off-by: Alexander Mohr <alexander.m.mohr@mercedes-benz.com
Side effect of this is, that the memory associated with the context now has to be managed in rust instead of C. Signed-off-by: Alexander Mohr <alexander.m.mohr@mercedes-benz.com>
makes enabling the trace load feature more convienient Signed-off-by: Alexander Mohr <alexander.m.mohr@mercedes-benz.com>
Makes it more convenient to create a DLtId instance from a string slice. Signed-off-by: Alexander Mohr <alexander.m.mohr@mercedes-benz.com>
2848e08 to
9a206e4
Compare
Signed-off-by: Alexander Mohr <alexander.m.mohr@mercedes-benz.com>
Add individual README files for dlt-sys, dlt-rs, and tracing-dlt crates to support publishing to crates.io. Each README provides crate-specific documentation, examples, and usage guidance. Individual READMEs make sure, that the published artifact on crates.io contains the correct documentation. Update root README to serve as workspace overview with table of crates and links to individual documentation. Signed-off-by: Alexander Mohr <alexander.m.mohr@mercedes-benz.com>
|
Added one more commit to split the readme files up, otherwise we wouldhave had an empty readme on crates.io (because readme key was not set in cargo.toml) |
tracing-dlt/src/lib.rs
Outdated
| let get = |i| bytes.get(i).copied().ok_or(DltError::InvalidInput); | ||
|
|
||
| match len { | ||
| 1 => DltId::new(&[get(0)?]), | ||
| 2 => DltId::new(&[get(0)?, get(1)?]), | ||
| 3 => DltId::new(&[get(0)?, get(1)?, get(2)?]), | ||
| _ => DltId::new(&[get(0)?, get(1)?, get(2)?, get(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.
Couldn't this be simplified to:
DltId::new(bytes.get(0..len).ok_or(DltError::InvalidInput)?)u8 should be automatically copied without the call to .copied() and the len is clamped to 1..4 anyways.
By using the get with a range we have the same safety as using the single gets I think.
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.
That doesn't work with the current signature of DltID::new because it expects a fixed size array.
pub fn new<const N: usize>(bytes: &[u8; N]) -> Result<Self, DltError> {
But the signature is only like this because I tried to evaluate the size at compile time through making new a const fn. This didn't work like I assumed initially, so it doesn't make sense to keep the current signature.
- Changing the signature to
pub fn new(bytes: &[u8]) -> Result<Self, DltError> { - Removed the function in tracing you've commented on and added DltId::from_str_clamped instead, this is using the suggestion simplified init.
- (Adjusted usages)
Simplifies building the class and adds util function. Signed-off-by: Alexander Mohr <alexander.m.mohr@mercedes-benz.com>
theswiftfox
left a comment
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
Thank you. I'll merge without squashing. The docs where removed in the commit which was introducing it, so it's not part of the history at all. |
Summary
Add the soruce code, CI, readme etc for the DLT libraries.
Please note that the generated documentation is commited on purpose.
This way it can be viewed in a browser without downloading, building locally or publishing on crates.io.
See
Only caveat right now is that linking between the two crates does not work.
Checklist
Related
Notes for Reviewers
Alexander Mohr alexander.m.mohr@mercedes-benz.com, Mercedes-Benz Tech Innovation GmbH
Provider Information