makefile: check cargo version #1503
Conversation
For the build-package target (used by cargo make repo), we need Cargo version 1.51 or higher to build a dependency of the variants workspace. To make this clear, we check the Cargo version before build-package and provide a clear error message.
| strarr=(${cargo_version//./ }) | ||
| cargo_major="${strarr[0]}" | ||
| cargo_minor="${strarr[1]}" | ||
| if [ "${cargo_major}" -gt "1" ] ; then |
There was a problem hiding this comment.
nit. It would be nice if we could standardize on [[...]].
nit. I don't think you need quotes for things without spaces (ie. numbers)
There was a problem hiding this comment.
This is typically done to maintain some level of compatibility with other shells, and then only use [[ ]] where you know you need bash specifics.
There was a problem hiding this comment.
If we are worried about compatibility with other shells, then shouldn't we avoid all instances of bash-isms?
There was a problem hiding this comment.
No, we do need some of them. It's a balance - sometimes bash-isms are the best way to do something, but otherwise staying compatible with POSIX sh and BSD userland is a good habit. We've seen both of those things bite us in Makefile.toml, for example. Even if your shebang is bash, it's nice to use sh basics when they're essentially identical, like in this case, in case you need the script to run elsewhere someday. (I'm old, and so have run into this many times.)
Revert back to where we were when we had a bunch of green check marks :) |
Issue number:
Follow up to #1408
Follow up to #1361
Description of changes:
Testing done:
rustup default 1.51.0 cargo make repo # Works!Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.