Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a continuous integration workflow that validates Slice file syntax by compiling them using the slicec compiler from the icerpc/slicec repository. The workflow runs on pushes and pull requests to the main branch.
Changes:
- Adds a CI workflow that checks out both the icerpc/slicec compiler and the current repository, then compiles all .slice files to verify syntax
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/ci.yml
Outdated
| path: slicec | ||
|
|
||
| - name: Checkout this repository | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
The checkout action version is inconsistent with the existing codebase convention. The spellcheck workflow uses actions/checkout@v4, but this workflow uses v3. Using v4 ensures access to the latest features and security updates.
.github/workflows/ci.yml
Outdated
|
|
||
| - name: Compile Slice files | ||
| working-directory: slicec | ||
| run: cargo run --release --bin slicec -- $(find ../icerpc-slice -name "*.slice") > /dev/null |
There was a problem hiding this comment.
We will only notice that we break things, if we do it the other way around too.
This too me sounds like slicec and slice should have a shared home.
There was a problem hiding this comment.
These Slice files shouldn't be used to test against compiler regressions.
There was a problem hiding this comment.
But it is not only regressions that will trigger this. For example if we drop a feature in slicec main, we need to do it here too. They are tie in that sense.
There was a problem hiding this comment.
Yes, these files are tied together, but it's not just slicec that is tied to this. Both slicec and slicec-cs depend on these files, and all 3 of these things need to be kept in sync.
Also, from a sub-tree perspective, it's easier for languages to only need to sub-tree one repo to get all the Slice files it needs, instead of those files being split in 2 places.
There was a problem hiding this comment.
For example if we drop a feature in slicec main, we need to do it here too. They are tie in that sense.
So, there's really two different things here, and unfortunately it's all kind of confusing to explain.
- There is making sure the Slice syntax in these files is correct
- There is making sure the Slice definitions in these files match
slicec's grammar
The CI is only to check #1. There is no good way to check #2 with only CI.
For example, let's say we dropped compact struct. This actually has no effect on these files being able to compile, since we don't use compact structs. CI would be okay, since the syntax is still valid.
But, we would need to remove the isCompact bool from struct Struct, so the definitions are "correct".
This is separate, and not something CI could check for us easily.
The "chicken" and the "egg" are really separate I think.
It's just hard to think about because the definitions of the Slice language are written in Slice : v)
| @@ -0,0 +1,28 @@ | |||
| name: CI | |||
|
|
|||
There was a problem hiding this comment.
I think is handy to allow to call this manually with workflow_dispatch, specially if you wan to test the new version.
|
I guess there is indeed a bit of a chicken and egg problem here. |
No description provided.