Closed SUPERCILEX closed 11 months ago
I’m not sure what your plan is to deal with builds that do not end up setting TRACY_ENABLE
which ends up producing the tracy client library without any symbols. I’m pretty sure we must have a way to not set that environment variable due to how just linking the tracy client libraries can begin broadcasting all sorts of information on the network.
Now one could argue that people could make tracy-client-sys
an optional dependency of their crates, but that’s a pretty onerous requirement to maintain given the possible implications of messing it up. On the other hand a disabled-by-default enable
feature is much more straightforward to control at the binary crate (with the libraries not needing to worry about it.)
For reference, I went ahead and ran the tests only to demonstrate the issues with the native symbols.
I’m not sure what your plan is to deal with builds that do not end up setting TRACY_ENABLE which ends up producing the tracy client library without any symbols
Hmmm, good point. So is the idea that people would use tracy-client-sys
as a library to build other crates? In that case, I think we can leave the enable feature in tracy-client-sys
and have it disabled by default (in fact all features should probably be disabled by default). Then, tracy-client and tracy-tracing (which I consider to be end user libraries that should be conditionally compiled in) always set the enable flag.
So the end result is:
tracy-client-sys
has all the features and full control.Removing the enable feature has the nice benefit of making sure that people who upgrade without reading the release notes will notice the behavior changed if they were using the enable feature.
Unfortunately, I do not believe the the downgrade in usability is going to cut it here. I agree that conceptually it is very nice to have the dependents do the proper thing and have the dependencies themselves optional. In practice I don’t think having every invocation of tracy_client::span!()
be wrapped in { #![cfg(feature = "tracy-client")] ... }
is going to make users happy. However if they don’t mind putting in that sort of effort, there's nothing stopping them from doing so already.
On the other hand, I personally found that handling enable
in the implementation of tracy-client
and especially tracing-tracy
to require minimal effort. Definitely not a sufficient pain point to eschew the benefits these features provide to the users of these crates.
Ah, shoot I forgot about people using plain spans instead of the tracing ones. Bummer, I think you're right that this isn't worth it.
This might be a controversial change (and it is breaking), but I think the enable feature is spreading bad practices. If a library/binary uses this crate's enable feature, it is still bringing in all the dependencies and build time to set up this crate only to have it be mostly empty. The correct way to use this crate would be to have a tracing feature in user's crates that conditionally enables this one, that way no trace (pun intended) of this crate or its dependencies remains in a production build.
Furthermore, for devs of this crate obeying the enable feature complicates reasoning around structs and the code in general.