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

makefile: check cargo version #1503

Merged
merged 1 commit into from
Apr 21, 2021
Merged

makefile: check cargo version #1503

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion Makefile.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,9 +266,31 @@ cargo install \
'''
]

# We need Cargo version 1.51 or higher in order to build a workspace's
# dependency during build-package
[tasks.check-cargo-version]
script_runner = "bash"
script = [
'''
set -euo pipefail
cargo_version=$(cargo --version | awk '{print $2}')
strarr=(${cargo_version//./ })
cargo_major="${strarr[0]}"
cargo_minor="${strarr[1]}"
if [ "${cargo_major}" -gt "1" ] ; then
Copy link
Member

Choose a reason for hiding this comment

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

nit. It would be nice if we could standardize on [[...]].
nit. I don't think you need quotes for things without spaces (ie. numbers)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is typically done to maintain some level of compatibility with other shells, and then only use [[ ]] where you know you need bash specifics.

Copy link
Member

Choose a reason for hiding this comment

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

If we are worried about compatibility with other shells, then shouldn't we avoid all instances of bash-isms?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.)

# cargo is version 2 or higher, so it's higher than 1.51
exit 0
fi
if [ "${cargo_minor}" -lt "51" ] ; then
echo "Error: Cargo 1.51.0 or greater is required, your version is ${cargo_version}" >&2
exit 1
fi
'''
]

# Builds a package including its build-time and runtime dependency packages.
[tasks.build-package]
dependencies = ["build-tools", "publish-setup"]
dependencies = ["check-cargo-version", "build-tools", "publish-setup"]
script_runner = "bash"
script = [
'''
Expand Down