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.

Sign up for GitHub

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

Jump to bottom

Versioning diplomat-tool through Cargo #4197

Merged
merged 7 commits into from
Oct 21, 2023
Merged

Versioning diplomat-tool through Cargo #4197

merged 7 commits into from
Oct 21, 2023

Conversation

robertbastian
Copy link
Member

@robertbastian robertbastian commented Oct 20, 2023 •
edited
Loading

This is cleaner than relying on the globally installed diplomat-tool being the correct version, and allows for faster iteration.

Even after cargo clean, cargo run -p diplomat-gen is faster than cargo install diplomat-tool && diplomat-tool, and with incremental builds the rebuild is negligible but guarantees the correct version.

Uses rust-diplomat/diplomat#348

@robertbastian robertbastian requested review from Manishearth, sffc and a team as code owners October 20, 2023 12:11
@robertbastian robertbastian force-pushed the dipl-tool branch 6 times, most recently from 1473328 to 3bba7f8 Compare October 20, 2023 12:33
@robertbastian robertbastian mentioned this pull request Oct 20, 2023
Closed
@Manishearth
Copy link
Member

Not a huge fan since diplomat gets reinstalled once every few months, whereas I end up cleaning somewhat regularly because our feature combinations take up disk space, and diplomat-gen is something I need to run on most PRs.

@robertbastian
Copy link
Member Author

since diplomat gets reinstalled once every few months

I want to iterate faster than that.

diplomat-gen is something I need to run on most PRs

It takes around 10 seconds for a clean build.

@Manishearth
Copy link
Member

It takes around 10 seconds for a clean build.

Ah. I feel like it takes longer to build but perhaps that's release mode. That's fine then

@robertbastian
Copy link
Member Author

Yeah it takes minutes in release mode, which is why I don't want to keep reinstalling it that way.

Manishearth
Manishearth reviewed Oct 21, 2023
View reviewed changes
tools/make/ffi.toml
# min version for cargo-make itself). But cargo-make might only be applying
# such dep upgrades defined in the `install_crate` field after the task script
# has run.
[tasks.cargo-make-min-version]
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd like to keep these around, being able to diplomat-install is still somewhat convenient during debugging

Copy link
Member Author

Choose a reason for hiding this comment

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

But we don't need diplomat-tool anymore

Manishearth
Manishearth previously approved these changes Oct 21, 2023
View reviewed changes
@robertbastian robertbastian merged commit 41ba05a into unicode-org:main Oct 21, 2023
27 checks passed
@robertbastian robertbastian deleted the dipl-tool branch October 21, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Manishearth Manishearth Manishearth approved these changes

@sffc sffc Awaiting requested review from sffc sffc is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robertbastian @Manishearth