-
Notifications
You must be signed in to change notification settings - Fork 15
[Versioning] Rename datadog-profiling crates #1325
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
…e new libdd prefix
morrisonlevi
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.
Some small comments that may or may not to be resolved, but overall it looks good.
| default = ["ddcommon-ffi"] | ||
| cbindgen = ["build_common/cbindgen", "libdd-common-ffi/cbindgen"] | ||
| ddtelemetry-ffi = ["dep:libdd-telemetry-ffi"] | ||
| libdd-telemetry-ffi = ["dep:libdd-telemetry-ffi"] # package_ffi_on_windows needs 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.
Was this intentionally dropped?
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.
This is weird, it was needed by the gitlab workflow when I renamed libdd-telemetry-ffi because windows build were unable to find the ddtelemetry-ffi feature and I duplicated it.
But now it runs without problems, so idk what have changed there... I'm taking a look.
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.
Ok, mystery solved, @hoolioh fixed 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.
We kind of stepped in each other toes. We changed the feature name to the new name due to the windows build was failing and then I realized that it was a mistake and fix the windows build script itself
libdd-profiling-ffi/src/exporter.rs
Outdated
| assert_eq!(internal["execution_trace_enabled"], "false"); | ||
| assert_eq!(internal["extra object"], json!({"key": [1, 2, true]})); | ||
| assert!(internal["libdatadog_version"].is_string()); | ||
| assert!(!internal["libdatadog_version"].as_str().unwrap().is_empty()); |
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.
Not necessarily a blocker, but we talked about using either version, whether the profiling or profiling-ffi version as they should be the same. This tests that it's not empty, which isn't the same.
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 I remember we solved that in the original PR but I didn't port the change to this branch. I'll port the changes here. The solution was to use the ffi package version.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What does this PR do?
Rename datadog-profiling crates according to the new naming convention.
Motivation
Prepare datadog-profiling to be published.