-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Build Sway projects at every CI run #234
Conversation
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.
Just remove the swayswap project and add some newlines, otherwise great work!!! This is going to be awesome to have!
packages/fuels-abigen-macro/tests/test_projects/contract_output_test/Forc.lock
Outdated
Show resolved
Hide resolved
packages/fuels-abigen-macro/tests/test_projects/swayswap/swayswap_abi/Forc.toml
Outdated
Show resolved
Hide resolved
packages/fuels-abigen-macro/tests/test_projects/swayswap/swayswap_abi/src/main.sw
Outdated
Show resolved
Hide resolved
You could consider using the somewhat idiomatic https://github.com/matklad/cargo-xtask With cargo aliasing, you get a nice cli interface like We use it in fuel-core and it works great. |
That'd be a nice idea to explore in future PRs -- I have no idea how it works and it would require some exploration time on my end, plus:
For now, I'd rather follow how it's done in sway, push this out, and unblock many things that are waiting for the next release. |
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 🎉 🎉
.github/workflows/ci.yml
Outdated
uses: actions-rs/cargo@v1 | ||
with: | ||
command: install | ||
args: forc |
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.
You should fix a forc
version here instead of using latest
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.
Wait isn't it going to be problematic? I feel like the whole point was to use the last compiler version (ie last forc
version) so we shouldn't pin it? What am I missing?
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.
Yeah, ideally we want to test it against the latest. But, for instance, if for some reason we don't wanna use the latest (e.g. latest breaks something on the SDK, we're aware of it, but we're still working on a fix), we might not want to block other PR's CI pipeline because of it. So having this control is nice.
But, then again, we have to watch ourselves and make sure we're updating this version when there's a new forc
version.
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.
If we want the best of both worlds, then we can create a second CI workflow that runs against latest
forc
and isn't required to pass to merge PRs. Then if it breaks we know, but it's not blocking anything.
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.
Ok, I feel like we should find a way to ensure this? I think we can't afford finding out once again that some tests are broken with 2 releases late? The SDK is getting quite big haha
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.
Not a huge fan of that because I believe it would increase the CI runtime considerably :(. We're currently sitting at ~10min, running another one, in this case, would mean building all Sway projects again.
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.
maybe the solution would be to run the fuels-rs
CI on every forc
major release?
But just on releases?
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.
Ah no wait that doesn't work
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.
above
This PR:
harness.rs
test suite;gitignore
s to not include lock files, ABI files, and binary files;cargo run --bin build-test-projects
). This needs to be run locally when running tests;forc
, at every new CI run, guaranteeing fresh binaries when running the test suite.Closes #210.
Closes #201.