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

Rename ddprof-ffi crate to datadog-profiling-ffi #39

Merged
merged 6 commits into from
Aug 19, 2022

Conversation

morrisonlevi
Copy link
Contributor

@morrisonlevi morrisonlevi commented Aug 18, 2022

What does this PR do?

  • This renames the ddprof-ffi crate to datadog-profiling-ffi. It renames the folder accordingly.
  • Format cmake files and add Secur32 and Ncrypt libs on Win. Move libs into main cmake folder (instead of the examples) so anyone using the cmake files get these linked.
  • Clean up ffi examples a bit.
  • Bumps version to 0.8.0-alpha.1.

Motivation

Before going 1.0, we wanted to do a pass on naming.

Additional Notes

Originally, this PR moved the ffi crate into the profiling crate as a feature-enabled module. Although I liked this better, the file sizes increased 50-70% because of an LTO bug in rust: rust-lang/rust#51009.

How to test the change?

The pkg-config and cmake files names changed, as did the name of the packages. Once those are fixed, upgrading should be pretty seamless.

@morrisonlevi morrisonlevi marked this pull request as ready for review August 18, 2022 19:06
@morrisonlevi morrisonlevi requested review from a team as code owners August 18, 2022 19:06
The feature is not enabled by default and is named
`datadog_profiling_ffi`.

I'm not 100% sure this is actually better. cbindgen maps the feature
into a C define, which requires users to set -DDATADOG_PROFILING_FFI
when compiling. I've adjusted the pkg-config and cmake helpers to do
this.

The upside is that it's more contained, and the artifacts are named
libdatadog_profiling.so instead of libdatadog_profiling_ffi.so. It's
also less lines in the repo.

Update licenses

Format cmake files and add Secure32 on Win

Also move the list of libraries that need linked into the source
cmake instead of the example.

Clean up ffi examples a bit
Due to a [rust bug][1], LTO was not being applied, and we need the lib
crate type in order to run tests and clippy lints. For now, keep these
as separate crates.

  [1]: rust-lang/rust#51009
@morrisonlevi morrisonlevi changed the title Move ddprof-ffi into profiling crate as a feature Rename ddprof-ffi crate to datadog-profiling-ffi Aug 19, 2022
Copy link
Contributor

@pawelchcki pawelchcki left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

cmake/DatadogConfig.cmake.in Outdated Show resolved Hide resolved
profiling-ffi/datadog_profiling-static.pc.in Outdated Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@nsavoire nsavoire left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the cleanup !

@morrisonlevi morrisonlevi merged commit 21c7ce3 into main Aug 19, 2022
@morrisonlevi morrisonlevi deleted the levi/bye-ddprof-ffi branch August 19, 2022 14:38
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Late review, but it LGTM 👍

I'm soft-concerned about the file size increase, but will measure and report in the channel if there's any concerns separately :)

Update: I misread that the file size increase change was still left in, and actually it hasn't.

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.

4 participants