-
Notifications
You must be signed in to change notification settings - Fork 57
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
Rewrite the update script in Python #112
Conversation
With various updates to this script such as having a different upstream for `memory64` and otherwise including the `wasm-3.0` test suite this script was getting relatively complicated which is typically not a great situation for any bash script to be in. Instead rewrite this script in Python to avoid many of bash's pitfalls and be a bit more readable/reviewable/modifiable.
Inspired by discussion on #110 |
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 for working on this!
lgtm % a whole bunch of python nits.
Thanks for the review! I also fixed an issue where if the spec repo itself had changed tests a commit wasn't made, but that should be fixed now too. (also apologies for the "clearly I'm not a Python programmer" python) |
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.
lgtm
@sunfishcode are you OK with this change?
Repo('relaxed-simd'), | ||
Repo('custom-page-sizes'), | ||
Repo('wide-arithmetic'), | ||
] |
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 think it would be great if this list could be a purely declarative list of strings/data, but I guess since we this dependency between elements is probably easier this way.
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.
LGTM, thanks for doing this 🎉
I haven't reviewed the code myself yet, but I have no special concerns here, and I trust the judgement of the people who have already reviewed it. |
Ok I'm going to go ahead and merge this and I'll be on the hook for any failures that happen in the near future from this refactoring... |
Oh wow it's significantly faster than before! The last auto-update run was 5.5 minutes and a manually triggered run was 0.5 minutes. Looks like stdout/stderr are interleaved there which isn't great though, but otherwise it seems to have at least run successfully in CI which is nice. |
With various updates to this script such as having a different upstream for
memory64
and otherwise including thewasm-3.0
test suite this script was getting relatively complicated which is typically not a great situation for any bash script to be in. Instead rewrite this script in Python to avoid many of bash's pitfalls and be a bit more readable/reviewable/modifiable.