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

Fix CI workflow by using locally checked out reactor-ts #157

Merged
merged 12 commits into from
Jun 30, 2023

Conversation

axmmisaka
Copy link
Collaborator

@axmmisaka axmmisaka commented May 28, 2023

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 always lf-lang/reactor-ts......

@axmmisaka axmmisaka changed the title Fix workflow reference with checkout Fix CI workflow by using locally checked out ts reactor May 28, 2023
@axmmisaka axmmisaka requested a review from lhstrh May 28, 2023 06:33
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@axmmisaka
Copy link
Collaborator Author

I'm thinking naming the checked-out one as reactor-ts-external, would that be better?

@axmmisaka
Copy link
Collaborator Author

For test result please see axmmisaka#9.
I think CI/CD of this PR is running against CI/CD config of master of this repo which doesn't quite make sense......

@lhstrh lhstrh changed the title Fix CI workflow by using locally checked out ts reactor Fix CI workflow by using locally checked out reactor-ts May 28, 2023
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@axmmisaka
Copy link
Collaborator Author

Oh no, actions/checkout will purge the directory and then pull...... This needs some major change I think

@axmmisaka
Copy link
Collaborator Author

axmmisaka commented May 29, 2023

So now it will first checkout to ./lingua-franca then files will be moved to ./ (because the main repo's gh action is set up to only be run at pwd).
Let's see if this works......

axmmisaka#19

@lhstrh
Copy link
Member

lhstrh commented May 29, 2023

So now it will first checkout to ./lingua-franca then files will be moved to ./ (because the main repo's gh action is set up to only be run at pwd). Let's see if this works......

axmmisaka#19

Hm, I don't think this is necessary. Could can just do two different checkouts, each in their own directory. For the ./gradlew command, you just specify a current working directory for the step. I don't think we should be copying files around...

@axmmisaka
Copy link
Collaborator Author

axmmisaka commented May 29, 2023

So now it will first checkout to ./lingua-franca then files will be moved to ./ (because the main repo's gh action is set up to only be run at pwd). Let's see if this works......
axmmisaka#19

Hm, I don't think this is necessary. Could can just do two different checkouts, each in their own directory. For the ./gradlew command, you just specify a current working directory for the step. I don't think we should be copying files around...

I tried it and it does not work, reason not being ./gradlew needs to be run on pwd but instead install-rti task in lingua-franca needs to be run in pwd (because of the way it is designed)

https://github.com/lf-lang/lingua-franca/blob/09151c2ba867de90c0dcb6335f513b54c8ac128a/.github/actions/install-rti/action.yml#L6

and a working-directory cannot be specified for use. I thought about modifying CICD in the main branch but this is such a corner use case and I do not want to make huge modifications.
https://github.com/lf-lang/lingua-franca/blob/master/.github/actions/install-rti/action.yml

@lhstrh
Copy link
Member

lhstrh commented May 29, 2023

I'm 100% sure you can specify the directory for a command using the working-directory attribute that I shared a link to. Have you tried using that?

@lhstrh
Copy link
Member

lhstrh commented May 29, 2023

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.

Copy link
Member

@lhstrh lhstrh left a 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.

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@axmmisaka
Copy link
Collaborator Author

axmmisaka commented May 29, 2023

Sorry for the long wait, I tried this before moving to the current non-elegant solution (axmmisaka@b453271) and here is the issue:
While we can invoke ./lingua-franca/.github/actions/install-rti, the way it is designed is not very compatible: it calls ./.github/actions/install-rti/install.sh and we cannot set its pwd here, resulting in file not found error if the file is instead located in lingua-franca/.github/actions/install-rti/install.sh
https://github.com/lf-lang/lingua-franca/blob/master/.github/actions/install-rti/action.yml

@lhstrh
Copy link
Member

lhstrh commented May 29, 2023

Sorry for the long wait, I tried this before moving to the current non-elegant solution (axmmisaka@b453271) and here is the issue: While we can invoke ./lingua-franca/.github/actions/install-rti, the way it is designed is not very compatible: it calls ./.github/actions/install-rti/install.sh and we cannot set its pwd here, resulting in file not found error if the file is instead located in lingua-frnca/.github/actions/install-rti/install.sh https://github.com/lf-lang/lingua-franca/blob/master/.github/actions/install-rti/action.yml

I see, but we're currently not calling the install-rti action? It's a trivial action, so we don't have to use it. We can also give it an input to specify where to find the sources...

@axmmisaka
Copy link
Collaborator Author

Sorry for the long wait, I tried this before moving to the current non-elegant solution (axmmisaka@b453271) and here is the issue: While we can invoke ./lingua-franca/.github/actions/install-rti, the way it is designed is not very compatible: it calls ./.github/actions/install-rti/install.sh and we cannot set its pwd here, resulting in file not found error if the file is instead located in lingua-frnca/.github/actions/install-rti/install.sh https://github.com/lf-lang/lingua-franca/blob/master/.github/actions/install-rti/action.yml

I see, but we're currently not calling the install-rti action? It's a trivial action, so we don't have to use it. We can also give it an input to specify where to find the sources...

I think we are referring to two actions defined in lingua-franca

- name: Install RTI
uses: ./.github/actions/install-rti

- name: Prepare build environment
uses: ./.github/actions/prepare-build-env

Both of which were built under the assumption that lingua-franca is at pwd.

I think the best way might be stick with the mv workaround for now, and edit the lf CI later, because I realised if people use lf, they might need some ways to set up lf in an alternate directory.

@axmmisaka
Copy link
Collaborator Author

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...

@axmmisaka
Copy link
Collaborator Author

Bug is fixed; before we fix this issue I think what we have right now is the best solution in terms of compatibility.
I recommend merge this after CICD is finished.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
Co-authored-by: Marten Lohstroh <marten@berkeley.edu>
@axmmisaka
Copy link
Collaborator Author

I think this is ready to merge? We should squash and merge though, because it contains too many garbage commits which are not needed.

@lhstrh lhstrh merged commit a1bd940 into lf-lang:master Jun 30, 2023
2 checks passed
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.

2 participants