-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix CI workflow by using locally checked out reactor-ts #157
Fix CI workflow by using locally checked out reactor-ts #157
Conversation
I'm thinking naming the checked-out one as |
For test result please see axmmisaka#9. |
Oh no, |
So now it will first checkout to |
Hm, I don't think this is necessary. Could can just do two different checkouts, each in their own directory. For the |
I tried it and it does not work, reason not being and a |
I'm 100% sure you can specify the directory for a command using the |
Oh, wait, you're talking about a different problem. Either way, I think that what you're doing now isn't necessary, but let's see whether it works before we refactor it. |
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.
How about this? Also, it occurs to me that this workflow should install the RTI, which it's currently not doing.
Sorry for the long wait, I tried this before moving to the current non-elegant solution (axmmisaka@b453271) and here is the issue: |
I see, but we're currently not calling the |
I think we are referring to two actions defined in reactor-ts/.github/workflows/ci.yml Lines 45 to 46 in 6d7c64b
reactor-ts/.github/workflows/ci.yml Lines 37 to 38 in 6d7c64b
Both of which were built under the assumption that I think the best way might be stick with the |
I'm working on this but lf tests are failing for some reason; I'll take some time to investigate and hopefully resolve by today as #163 is kinda dependent on it... |
Bug is fixed; before we fix this issue I think what we have right now is the best solution in terms of compatibility. |
Co-authored-by: Marten Lohstroh <[email protected]>
I think this is ready to merge? We should squash and merge though, because it contains too many garbage commits which are not needed. |
Merge either this or #156.
Right now, the CI/CD uses
"git://github.com/lf-lang/reactor-ts.git#${{ github.head_ref || github.ref_name }}"
as LF reactor runtime. This is not good because PRs from external repositories will fail the CICD test.This is one approach to fix it (which I like): checkout the reactor and let LF use that (the checked out) local reactor.
Squash and rebase merge this PR please. Do not just merge because it will look terrible.
I suppose https://github.com/lf-lang/reactor-ts/tree/lhstrh-patch-1 is trying to fix the same thing but sadly
${{ github.repositoryUrl }}
is still alwayslf-lang/reactor-ts
......