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.

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

Add --require-version flag #497

Closed
wants to merge 1 commit into from

Conversation

Timmmm
Copy link
Contributor

@Timmmm Timmmm commented Apr 17, 2024

This adds a flag that lets you require a minimum version of the Sail compiler. If the version doesn't meet the requirements it exits with exit code 1. Otherwise it continues as normal.

This means you can use it as a standalone check (sail --require-version 0.17.2) or as an additional flag to your existing commands.

I added --abbrev=0 to the git describe command so we don't have to later strip the stuff after the -.

@Timmmm
Copy link
Contributor Author

Timmmm commented Apr 17, 2024

Apologies for my OCaml! Especially helpful_int_of_string - presumably there is a better way.

Copy link

github-actions bot commented Apr 17, 2024

Test Results

    9 files  ±0     20 suites  ±0   0s ⏱️ ±0s
  619 tests ±0    619 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 984 runs  ±0  1 983 ✅ ±0  1 💤 ±0  0 ❌ ±0 

Results for commit f47222d. ± Comparison against base commit cb0a396.

♻️ This comment has been updated with latest results.

src/bin/sail.ml Outdated Show resolved Hide resolved
src/bin/sail.ml Outdated Show resolved Hide resolved
src/bin/sail.ml Outdated Show resolved Hide resolved
src/bin/sail.ml Outdated Show resolved Hide resolved
This adds a flag that lets you require a minimum version of the Sail compiler. If the version doesn't meet the requirements it exits with exit code 1. Otherwise it continues as normal.

This means you can use it as a standalone check (`sail --require-version 0.17.2`) or as an additional flag to your existing commands.

The Sail version is also now hard-coded, and verified against the Git tags in CI.
@arichardson
Copy link
Contributor

@Alasdair would be great to have a new release that includes this flag soon so we can update the riscv sail model build system to have fewer confusing failure modes.

@Alasdair
Copy link
Collaborator

Yep, I will take this and probably simplify the version logic a bit. I'll try to get it in tomorrow.

@Alasdair
Copy link
Collaborator

Ah so the reason the check in the CI fails is because GitHub actions does a shallow clone that doesn't include the tags. It's not showing as failing on this PR because the ci_core_tests script is only run for the build matrix job, and that isn't enabled for external PRs right now.

@Timmmm
Copy link
Contributor Author

Timmmm commented May 15, 2024

Ahh that also explains why the version was unknown in my binary release.

@Alasdair
Copy link
Collaborator

I might just remove that check in the action, just have to be careful to keep things in sync when bumping the version number.

@Timmmm
Copy link
Contributor Author

Timmmm commented May 15, 2024

I believe you can retrieve just the metadata history like this:

git fetch --unshallow --filter=tree:0

After I did that on a git clone --depth=1 <sail url> repo, then git describe worked.

@Alasdair
Copy link
Collaborator

I added this commit in #544, so closing this PR

I'll look at re-adding the version check in the CI separately

@Alasdair Alasdair closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants