-
-
Notifications
You must be signed in to change notification settings - Fork 315
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
Include cxxbridge-cmd in Cargo.lock, check version consistency #3712
Conversation
1b55aad
to
0451f7d
Compare
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.
Looks pretty good! Thanks for making the effort with that GitHub workflow.
.github/workflows/metadata-check.sh
Outdated
cxx_version=$(get_version "cxx") | ||
cxx_build_version=$(get_version "cxx-build") | ||
cxxbridge_cmd_version=$(get_version "cxx-build") | ||
cxxbridge_flags_version=$(get_version "cxxbridge-flags") | ||
cxxbridge_macro_version=$(get_version "cxxbridge-macro") | ||
|
||
ok=true | ||
echo "Found cxx version ${cxx_version}" | ||
if [ "${cxx_version}" != "${cxx_build_version}" ]; then | ||
echo "Found differing cxx-build version ${cxx_build_version}" | ||
ok = false | ||
fi | ||
if [ "${cxx_version}" != "${cxxbridge_cmd_version}" ]; then | ||
echo "Found differing cxxbridge-cmd version ${cxxbridge_cmd_version}" | ||
ok = false | ||
fi | ||
if [ "${cxx_version}" != "${cxxbridge_flags_version}" ]; then | ||
echo "Found differing cxxbridge-flags version ${cxxbridge_flags_version}" | ||
ok = false | ||
fi | ||
if [ "${cxx_version}" != "${cxxbridge_macro_version}" ]; then | ||
echo "Found differing cxxbridge-macro version ${cxxbridge_macro_version}" | ||
ok = false | ||
fi |
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.
For loop?
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.
Meh, maybe if we add more :)
Haven't tested this locally yet. |
This adds cxxbridge-cmd to Cargo.lock per dtolnay/cxx#1407 (comment) It adds an MSRV to `src/taskchampion-cpp/Cargo.toml` so that the version of `Cargo.lock` is stil compatible with the MSRV. It additionally adds a check of the Cargo metadata for all of the cxx* versions agreeing, and for the MSRV's agreeing.
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.
Of the 4 cxx version checks in metadata-check.sh, the only 1 that matters is not implemented correctly. That one should be fixed and the other 3 should be removed.
check_cxx_versions() { | ||
local cxx_version=$(get_version "cxx") | ||
local cxx_build_version=$(get_version "cxx-build") | ||
local cxxbridge_cmd_version=$(get_version "cxx-build") |
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.
local cxxbridge_cmd_version=$(get_version "cxx-build") | |
local cxxbridge_cmd_version=$(get_version "cxxbridge-cmd") |
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.
🤦🏼
echo "Found differing cxxbridge-flags version ${cxxbridge_flags_version}" | ||
ok = false | ||
fi | ||
if [ "${cxx_version}" != "${cxxbridge_macro_version}" ]; then |
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 already enforced by https://github.com/dtolnay/cxx/blob/1.0.124/Cargo.toml#L26.
echo "Found differing cxxbridge-cmd version ${cxxbridge_cmd_version}" | ||
ok = false | ||
fi | ||
if [ "${cxx_version}" != "${cxxbridge_flags_version}" ]; then |
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 already enforced by https://github.com/dtolnay/cxx/blob/1.0.124/Cargo.toml#L31.
|
||
ok=true | ||
echo "Found cxx version ${cxx_version}" | ||
if [ "${cxx_version}" != "${cxx_build_version}" ]; then |
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 already enforced by https://github.com/dtolnay/cxx/blob/1.0.124/Cargo.toml#L34.
if [ "${cxx_version}" != "${cxxbridge_cmd_version}" ]; then | ||
echo "Found differing cxxbridge-cmd version ${cxxbridge_cmd_version}" | ||
ok = false | ||
fi |
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.
I published cxx 1.0.133 which automatically puts a correct version of cxxbridge-cmd into the lockfile of any crate that depends on cxx (dtolnay/cxx#1408). Could you try upgrading, reverting the taskchampion-cpp/Cargo.toml change from this PR, and see if cxxbridge-cmd stays in the lockfile? If so then this check can also be removed.
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.
Thanks! I added #3714 to try this.
Per GothenburgBitFactory#3712 (review), this version includes a cxxbridge-cmd dependency internally, so there's no need to keep that within Taskwarrior, nor to check it in CI.
This adds cxxbridge-cmd to Cargo.lock per dtolnay/cxx#1407 (comment), as linked from #3705.
It adds an MSRV to
src/taskchampion-cpp/Cargo.toml
so that the version ofCargo.lock
is stil compatible with the MSRV.It additionally adds a check of the Cargo metadata for all of the cxx* versions agreeing, and for the MSRVs agreeing.
cc: @doronbehar