Add a Sail version check to Makefile

[Alasdair]

In https://github.com/riscv/sail-riscv/issues/368#issuecomment-1846210592 it was suggested to add a Sail version check to prevent errors where some Makefile invocation fails mysteriously due to a too old version of Sail.

I'm not sure what the best way to do this is: Here I've added a small shell script and a check at the top of the Makefile.

There are some pathological cases where a Sail binary might not know its own version, which would be when it's built from source without either git or opam. I don't think this is likely to ever happen, but I've added an option to skip the version check by setting an environment variable.

[github-actions[bot]]

Test Results

712 tests  ±0   712 :heavy_check_mark: ±0   0s :stopwatch: ±0s     6 suites ±0       0 :zzz: ±0      1 files   ±0       0 :x: ±0 

Results for commit 91d3cd6f. ± Comparison against base commit 393e7ed9.

[arichardson]

Would it be simpler to use something like the -has_feature flag in sail to require a given feature so we don't have to parse arguments? I guess not every release has new features so maybe the version check is better.

[Alasdair]

Yes, that's been fairly ad-hoc. If I was more forward-thinking I could have added a feature flag per-release, but I haven't.

I could add something like a --version-check flag, but of course that wouldn't serve the goal at first because the older versions we would want to check for wouldn't recognise that flag.

[arichardson]

I think it would be very useful to land this so we have a way of enforcing a minimum version of sail and won't see any more issues filed due to an incorrect version.

[Timmmm]

Perhaps to avoid janky Bash scripts Sail could do this check itself, something like --min-version 0.17 and it exits with a non-zero exit code if it is less than that version. This would be useful for anyone using Sail (not just this repo).

It does mean we need to wait a bit until the minimum version has that flag, but I'd say that's acceptable.

[arichardson]

Perhaps to avoid janky Bash scripts Sail could do this check itself, something like --min-version 0.17 and it exits with a non-zero exit code if it is less than that version. This would be useful for anyone using Sail (not just this repo).

It does mean we need to wait a bit until the minimum version has that flag, but I'd say that's acceptable.

Adding support to sail itself sounds like a good plan to me. Alternatively it could pre-define something like a SAIL_VERSION symbol that is checked by prelude.sail and we emit an error if it's insufficient.

[allenjbaum]

Or at least a warning?

On Monday, January 15, 2024, Alexander Richardson @.***> wrote:

Perhaps to avoid janky Bash scripts Sail could do this check itself, something like --min-version 0.17 and it exits with a non-zero exit code if it is less than that version. This would be useful for anyone using Sail (not just this repo).

It does mean we need to wait a bit until the minimum version has that flag, but I'd say that's acceptable.

Adding support to sail itself sounds like a good plan to me. Alternatively it could pre-define something like a SAIL_VERSION symbol that is checked by prelude.sail and we emit an error if it's insufficient.

— Reply to this email directly, view it on GitHub https://github.com/riscv/sail-riscv/pull/370#issuecomment-1892929434, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJVPJNUMIKONRZPPF7DYOXHGRAVCNFSM6AAAAABAL35F4OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJSHEZDSNBTGQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[Timmmm]

Adding support to sail itself sounds like a good plan to me. Alternatively it could pre-define something like a SAIL_VERSION symbol that is checked by prelude.sail and we emit an error if it's insufficient.

That's definitely better in that you can't accidentally forget to check it. On the other hand it won't give you a nice error if e.g. you add a new flag to the compiler that we want to use. You'll get Unknown flag --new-feature or whatever.

Another consideration is that you may want to use this mechanism to introduce backwards incompatible changes to the language, which you wouldn't be able to do with either of these solutions.

How about:

1. Add a min_sail_version key to the .sail_project file. This would be checked by the Sail compiler and could also be used to enable backwards incompatible features (e.g. if min_sail_version is >0.21 then implicit var is an error).
2. Add a sail --check-version '>=0.17' type option. This can optionally be used in build systems just to ensure that the user has a recent enough compiler before we start using new flags & features.

The second one would really just be nice to have in order to give nicer error messages. Maybe not worth the effort if we have the first one.

[arichardson]

I like @Timmmm's suggestion, but I think we should really land some kind of version check soon. No need to wait for a perfect solution if we can fix the most important issue with this PR.

I think this PR should be good to land with the shellcheck issues fixed.

[Timmmm]

I agree we should do this soon, but I also have an entirely rational hatred of Bash, so I've made a PR to add a version check flag to Sail: https://github.com/rems-project/sail/pull/497

[jordancarlin]

This can be closed now that #534 was merged instead.