OpenCyphal / pycyphal

Python implementation of the Cyphal protocol stack.
https://pycyphal.readthedocs.io/
MIT License
123 stars 106 forks source link

Issue 259 - Create GH Actions workflow to replace AppVeyor #303

Closed clyde-johnston closed 1 year ago

clyde-johnston commented 1 year ago

This PR addresses issue #259 to move the AppVeyor workflow to GH Actions. Some differences:

Important: The Windows self-hosted runners differ from the GH Actions runners in that they are not rebuilt after each workflow run. Therefore, there is a chance the Windows runners could become corrupted. A clean installation of the Windows runners can be made from AWS Images if required.

clyde-johnston commented 1 year ago

I have tested the PYPI_TEST_API_TOKEN_GLOBAL token on the testpypi repository and it works. I have amended the latest commit in this PR to use the PYPI_API_TOKEN_PYCYPHAL token on the pypi repository. This PR is ready to be merged.

Note: The GHA workflow will fail to deploy until the pycyphal version number is incremented.

clyde-johnston commented 1 year ago

The pull request event was added to the workflow as requested.

clyde-johnston commented 1 year ago

Note: The demo test fails on Ubuntu runners occasionally and randomly with a KeyboardInterrupt error. A rerun of the job usually resolves this.

clyde-johnston commented 1 year ago

I think one way to fix it is to add a condition like this:

if: github.event_name == 'push'

I do not think this is possible since the push and pull request events can be triggered singularly or together.

This is caused only when both a push and a pull request event are triggered together. It happens when you there is an open pull request like this one (#303) and then you push changes to the from branch (issue-259). But it does not happen if you create a pull request or if you push code any branch by itself.

The push test job gets cancelled because it runs exactly the same test as the pull request test job. I set this up in the concurrency rules (lines 4-7) to avoid duplicating the test jobs for both events. This is more important for the Windows runners since these are self-hosted and capacity constrained. It would double the time to complete the tests.

There are three possibilities:

  1. Accept that the push job will fail in the case where a push is made to a branch with an open pull request and both events are triggered together.
  2. Loosen the concurrency rules so that both tests are run for both events -- this will double the time to complete the Windows runner tests.
  3. Remove the trigger for the pull request so that the test results are drawn from the push event only -- I am not sure the test results will appear in the PR in this case.
pavel-kirienko commented 1 year ago

The solution I proposed is well-known and works as intended in other projects (e.g. see Dyshlo), so I suggest we go with it.

On Sun, Aug 13, 2023, 01:31 clyde-johnston @.***> wrote:

I think one way to fix it is to add a condition like this:

if: github.event_name == 'push'

I do not think this is possible since the push and pull request events can be triggered singularly or together.

This is caused only when both a push and a pull request event are triggered together. It happens when you there is an open pull request like this one (#303 https://github.com/OpenCyphal/pycyphal/pull/303) and then you push changes to the from branch (issue-259 https://github.com/OpenCyphal/pycyphal/tree/issue-259). But it does not happen if you create a pull request or if you push code any branch by itself.

The push test job gets cancelled because it runs exactly the same test as the pull request test job. I set this up in the concurrency rules (lines 4-7) to avoid duplicating the test jobs for both events. This is more important for the Windows runners since these are self-hosted and capacity constrained. It would double the time to complete the tests.

There are three possibilities:

  1. Accept that the push job will fail in the case where a push is made to a branch with an open pull request and both events are triggered together.
  2. Loosen the concurrency rules so that both tests are run for both events -- this will double the time to complete the Windows runner tests.
  3. Remove the trigger for the pull request so that the test results are drawn from the push event only -- I am not sure the test results will appear in the PR in this case.

— Reply to this email directly, view it on GitHub https://github.com/OpenCyphal/pycyphal/pull/303#issuecomment-1676127598, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZFIZG4ZFR3H44UC2GFEHLXU775ZANCNFSM6AAAAAAZBC47IE . You are receiving this because your review was requested.Message ID: @.***>

clyde-johnston commented 1 year ago

Could you please explain the purpose of the pull_request trigger as mentioned here in Slack? I am a bit confused this contradicts line 8 of Dyshlo. The Dyshlo workflow runs the test job only on the push trigger and the sonar job on the pull_request or push to main or release branches.

If the purpose is simply to display the test results in the PR for review before merging, then no pull_request trigger is needed since the PR automatically picks up the test results from the last commit.

I have removed the pull_request trigger and adjust the concurrency rule to demonstrate it here.

clyde-johnston commented 1 year ago

The ubuntu-latest image no longer works because it cannot find the linux-modules-extra-5.15.0-1042-azure package. It looks like there has been a change to the Ubuntu archives that requires a change to the GH image. I have reverted it back to ubuntu-20.04.

pavel-kirienko commented 1 year ago

Could you please explain the purpose of the pull_request trigger as mentioned here in Slack? I am a bit confused this contradicts line 8 of Dyshlo. The Dyshlo workflow runs the test job only on the push trigger and the sonar job on the pull_request or push to main or release branches.

This is because in Dyshlo, we don't want to run Sonar for every push to short-lived branches, as it is expensive. This is less relevant for PyCyphal so we can go with your solution for now.

However, at the moment Sonar does not appear to run; can you please check this? https://sonarcloud.io/project/overview?id=PyCyphal

clyde-johnston commented 1 year ago

The calls to Sonar and Coveralls in the PyCyphal workflow is now executing but they return the following errors.

Sonar

ERROR: Error during SonarScanner execution
ERROR: Project not found. Please check the 'sonar.projectKey' and 'sonar.organization' properties, the 'SONAR_TOKEN' environment variable, or contact the project administrator
ERROR: 
ERROR: Re-run SonarScanner using the -X switch to enable full debug logging.

Coveralls

Submitting coverage to coveralls.io...
Could not submit coverage: 422 Client Error: Unprocessable Entity for url: https://coveralls.io/api/v1/jobs
Traceback (most recent call last):
  File "/home/runner/work/pycyphal/pycyphal/.nox/test-3-10/lib/python3.10/site-packages/coveralls/api.py", line 290, in submit_report
    response.raise_for_status()
  File "/home/runner/work/pycyphal/pycyphal/.nox/test-3-10/lib/python3.10/site-packages/requests/models.py", line 1021, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 422 Client Error: Unprocessable Entity for url: https://coveralls.io/api/v1/jobs
pavel-kirienko commented 1 year ago

Re Sonar: please try changing the environment variable from SONARCLOUD_TOKEN to SONAR_TOKEN, as suggested in the error message. The properties mentioned there are already set correctly, so the problem is likely in the variable.

Re Coveralls: see https://github.com/lemurheavy/coveralls-public/issues/1435#issuecomment-763357004; also, we may need to export COVERALLS_REPO_TOKEN

clyde-johnston commented 1 year ago

I have tried changing the environment variable to SONAR_TOKEN but this did not resolve the issue. When I set the -X debug flag on the sonar-scanner, it reports:

java.lang.IllegalStateException: Unable to load component class org.sonar.scanner.bootstrap.GlobalConfiguration

It is very similar to this issue which was a results of "some sort of token mismatch". He resolved this by deleting and recreating the account in order to get a new token.

coveralls commented 1 year ago

Coverage Status

coverage: 95.357% (+0.005%) from 95.352% when pulling c56d1488d476ee5909a4b159f1affcef5caf5c27 on issue-259 into 01b9a9b57143bc916bfa2fa26102a787b4e558e9 on master.

clyde-johnston commented 1 year ago

Sonar and Coveralls are now working. This PR is ready for review.