Skip to content

Drop cargo configs#272

Merged
rustaceanrob merged 1 commit into2140-dev:masterfrom
nyonson:chore/tokio-console
Jan 17, 2025
Merged

Drop cargo configs#272
rustaceanrob merged 1 commit into2140-dev:masterfrom
nyonson:chore/tokio-console

Conversation

@nyonson
Copy link
Collaborator

@nyonson nyonson commented Jan 15, 2025

Instead of relying on dirty-ing a checked in file (.cargo/config.toml), switch to a feature flag based flow to enable the tokio debug console. This avoids accidental thrashing in the checked in file. It also allows a discoverable path from the justfile for new developers.

This is my first foray into the build.rs system, but it seems pretty straightforward. Also trying to be better about conventional commit messages.

Edit: Dropped all changes except for removing the cargo configs for now.

@rustaceanrob
Copy link
Collaborator

rustaceanrob commented Jan 15, 2025

I'm all in favor of simplifying the configurations in the repo. Having a look at this change there's a lot I don't quite understand. For one, since crate features are public I'm not a fan of having a feature that would only be useful for developers. If folks want to profile, they can bring what they need into their dependencies on their own. I think this is enough motivation to instead maintain a dev branch, and have it build the profiler by default. That way only developers actively working on the repository would have access to the console.

For this PR, can we 1. remove the .cargo folder 2. remove the comments on L61-63 in Cargo.toml 3. revert the other stuff like remove the just and the build script.

Thanks for drawing attention to this, should definitely not be in master.

@nyonson
Copy link
Collaborator Author

nyonson commented Jan 16, 2025

Good point on the public feature flags, didn't think of that. I'll dial this back to just dropping the config for now.

I am curious about a system that uses build.rs and maybe environment variable to enable certain developer features like the debug console. Then a just recipe could set them for the dev and it would have a nice discoverable interface. But feels a little much at the moment.

@nyonson nyonson force-pushed the chore/tokio-console branch from 337fb9a to a3bd008 Compare January 16, 2025 22:15
@nyonson nyonson changed the title Migrate tokio console control to build.rs Drop cargo configs Jan 16, 2025
@rustaceanrob
Copy link
Collaborator

There's also a couple comments in Cargo.toml on L61-63 that don't really apply now that we are scraping the .cargo configurations

Avoids accidental thrashing in the checked in file. If this is a
feature we want for development, we have a few ideas such as a
dev branch or leveraging the build.rs system. But currently not
enough demand.
@nyonson nyonson force-pushed the chore/tokio-console branch from a3bd008 to 698bf42 Compare January 17, 2025 19:20
@nyonson
Copy link
Collaborator Author

nyonson commented Jan 17, 2025

There's also a couple comments in Cargo.toml on L61-63 that don't really apply now that we are scraping the .cargo configurations

Oops, missed those. Cleaned up in recent commit.

@rustaceanrob
Copy link
Collaborator

ACK 698bf42

Thanks

@rustaceanrob rustaceanrob merged commit 8cffec6 into 2140-dev:master Jan 17, 2025
14 checks passed
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.

2 participants