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.

Sign up for GitHub

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

Jump to bottom

CI: Backport changes from master #3775

Merged
merged 19 commits into from
Apr 18, 2024
Merged

CI: Backport changes from master #3775

merged 19 commits into from
Apr 18, 2024

Conversation

imphil
Copy link
Member

@imphil imphil commented Mar 13, 2024

Backport all CI changes from master to get a running CI setup for
the stable/1.8 branch.

@marlonjames marlonjames added the category:ci continuous integration and unit tests label Mar 13, 2024
@codecov Codecov
Copy link

codecov bot commented Mar 13, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.34%. Comparing base (4afe0a4) to head (bfd8e95).

Additional details and impacted files 
@@              Coverage Diff               @@
##           stable/1.8    #3775      +/-   ##
==============================================
+ Coverage       60.23%   64.34%   +4.10%     
==============================================
  Files              49       49              
  Lines            8845     9115     +270     
  Branches         2466     2454      -12     
==============================================
+ Hits             5328     5865     +537     
- Misses           2438     2658     +220     
+ Partials         1079      592     -487

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@imphil
Copy link
Member Author

imphil commented Mar 14, 2024

Hm, the flake8 lint errors don't make any sense:

cocotb/config.py:67:26: E231 missing whitespace after ':'
cocotb/log.py:150:33: E231 missing whitespace after ':'
cocotb/log.py:213:38: E231 missing whitespace after ':'
cocotb/runner.py:624:25: E231 missing whitespace after ':'
cocotb/types/logic_array.py:163:53: E713 test for membership should be 'not in'
cocotb/types/logic_array.py:179:59: E713 test for membership should be 'not in'
examples/analog_model/afe.py:82:76: E231 missing whitespace after ':'
examples/mixed_signal/tests/test_rescap.py:139:46: E231 missing whitespace after ':'
examples/mixed_signal/tests/test_rescap.py:146:50: E231 missing whitespace after ':'
tests/test_cases/issue_330/issue_330.py:25:62: E702 multiple statements on one line (semicolon)
tests/test_cases/test_cocotb/test_testfactory.py:41:51: E231 missing whitespace after ':'

The commit that's tested is https://github.com/cocotb/cocotb/blob/6cf80c02b40af81ca0b038e2f0298349870e97ea/cocotb/types/logic_array.py, and in line 163 I don't see any membership test that would trigger flake8. I also don't see anything like these messages locally ... More investigation needed.

@ktbarrett
Copy link
Member

flake8 is losing its damn mind... checking the contents of strings...

@imphil imphil closed this Apr 15, 2024
@imphil imphil reopened this Apr 15, 2024
@ktbarrett
Copy link
Member

ktbarrett commented Apr 15, 2024 •
edited

The failing test passes with pytest<7. There was a change in behavior that causes a different exception to be raised if an exception is raised in a pytest.warns block.

I suggest changing the test to skip Icarus since that element being tested is ifdef'd out of the SV source for Icarus.

@imphil
Copy link
Member Author

imphil commented Apr 15, 2024

Ah, it's pytest that changed ... I was about to loose my mind over it. (The beauty of not pinning versions.) Thanks!

Do we have a change on master that fixes the test? (I couldn't find it, but now at least I have a better idea how to locally reproduce what I'm seeing in CI.)

@ktbarrett
Copy link
Member

@imphil That test was deleted on master because it was for deprecated (now removed) functionality.

@imphil
Make test_deprecated work with pytest 8
03fc611 
Pytest 8 introduced new behavior for `pytest.warn()`, which isn't
compatible with our code in `test_unicode_handle_assignment_deprecated`.
Update the test to be skipped entirely for Icarus and GHDL instead of
testing for specific exceptions. (The test doesn't do anything on these
simulators anyways.)

See also pytest-dev/pytest#11129

Error message, as seen in CI:
```

     1.00ns INFO     cocotb.regression                  running test_unicode_handle_assignment_deprecated (2/195)
     1.00ns INFO     cocotb.regression                  test_unicode_handle_assignment_deprecated failed: errored with unexpected type
  Traceback (most recent call last):
    File "/home/runner/work/cocotb/cocotb/tests/test_cases/test_cocotb/test_deprecated.py", line 39, in test_unicode_handle_assignment_deprecated
      dut.stream_in_string.value = "Bad idea"
    File "/home/runner/work/cocotb/cocotb/.nox/dev_test_sim-sim-icarus-toplevel_lang-verilog-gpi_interface-vpi/lib/python3.8/site-packages/cocotb/handle.py", line 370, in __getattr__
      raise AttributeError(f"{self._name} contains no object named {name}")
  AttributeError: sample_module contains no object named stream_in_string

  During handling of the above exception, another exception occurred:

  Traceback (most recent call last):
    File "/home/runner/work/cocotb/cocotb/tests/test_cases/test_cocotb/test_deprecated.py", line 39, in test_unicode_handle_assignment_deprecated
      dut.stream_in_string.value = "Bad idea"
    File "/home/runner/work/cocotb/cocotb/.nox/dev_test_sim-sim-icarus-toplevel_lang-verilog-gpi_interface-vpi/lib/python3.8/site-packages/_pytest/recwarn.py", line 322, in __exit__
      fail(
    File "/home/runner/work/cocotb/cocotb/.nox/dev_test_sim-sim-icarus-toplevel_lang-verilog-gpi_interface-vpi/lib/python3.8/site-packages/_pytest/outcomes.py", line 166, in fail
      raise Failed(msg=reason, pytrace=pytrace)
  Failed: DID NOT WARN. No warnings of type (<class 'DeprecationWarning'>,) were emitted.
    Emitted warnings: [].
```
@imphil
Copy link
Member Author

imphil commented Apr 17, 2024

RTD failed with AttributeError: module 'sphinx.domains.cpp' has no attribute 'ASTParametersQualifiers', which is probably resolved by sphinx-doc/sphinx#12298 in https://github.com/sphinx-doc/sphinx/releases/tag/v7.3.6 (released 3 minutes ago, just in time!)

@imphil
CI: Enable tests for Python 3.13
1f3bef2 
(cherry picked from commit 35114fc)
@imphil
CI: Use the latest icarus release
486cbfd 
v12 of Icarus Verilog went out in June. Update to this version, which is
also newer than the git commit we used in some places.

(cherry picked from commit 8015f1d)
@imphil
CI: Include the toplevel language in the job name
2fd3c3d 
Depending on the toplevel language, we're running a very different set
of tests. Include it in the name to make the tests unambiguous.

While at it, refactor the code a bit to
* place the generation of the human-readable job name into the Python
  script
* reuse the name in the Codecov upload, instead of assembling it twice
  (we can do that now that we have the name in the matrix configuration)
* consistently use hyphens as delimitter in matrix env keys
* shorten the simulator version number by removing the module name
  prefix (e.g. 'siemens/questa/2023.2' to '2023.2')
* shorten the name by removing spaces around the pipe symbol.

A shorter name helps to display more of it in the left navigation bar of
the GH Checks view.

(cherry picked from commit 866df6f,
plus formatting updates for this branch)
@imphil
CI: Update to Questa 2023.4_1
160c074 
(cherry picked from commit 46f570d)
@imphil
Copy link
Member Author

imphil commented Apr 17, 2024 •
edited

The failing test passes with pytest<7. There was a change in behavior that causes a different exception to be raised if an exception is raised in a pytest.warns block.

It's pytest 8 that introduces the breakage. That also explains why the Icarus + Python 3.6 and 3.7 tests were passing: pytest 7 dropped Python 3.6 support, and pytest 8 dropped Python 3.7 support. Those two Python versions were picking up older pytest versions, which made the tests pass.

I suggest changing the test to skip Icarus since that element being tested is ifdef'd out of the SV source for Icarus.

Yes, that's the approach I went for.

@imphil
Add test waivers for Riviera-PRO struct and unions in VPI
83fa719 
See cocotb#3587 for a detailed
description of why these waivers are necessary.

(cherry picked from commit 0a9b372)
@imphil
Remove test waiver for test_both_edge_triggers on recent Riviera
188e9a0 
Riviera 2023.04 has fixed the underlying issue that prevented
test_both_edge_triggers to run in this simulator, remove the test
exclusion.

References: SPT81300 (Aldec)
cocotb#2344 (cocotb)
(cherry picked from commit 5cdf26c)
@imphil
Use the latest version of Riviera in CI
b695c52 
(cherry picked from commit c7347b3)
@imphil
CI: Use distinct labels for private jobs
dfad5c6 
Labels in GitHub Actions are used to determine which runner can execute
a CI job. With our setup of self-hosted and GitHub-hosted runners,
labels decide if a job gets to run on our own infrastructure, or on
GitHub infrastructure.

Right now, we're assinging the labels "self-hosted", "ubuntu-20.04",
"cocotb-private", "X64" and "Linux" to all self-hosted runners.

All jobs that we want to run on our self-hosted runners get the labels
"self-hosted", "cocotb-private", and "ubuntu-20.04" assigned.

Jobs that we want to run on the GitHub-provided runners only get a
single label, the operating system, such as "ubuntu-20.04".

As it turns out, a job gets executed on a particular runner as soon as
all labels assigned *to the job* are matching labels assigned to the
runner.

For us, this meant a job assigned only the label "ubuntu-20.04" (a job
which we want to execute on GitHub-provided runners) got executed on
our self-hosted runners, since "ubuntu-20.04" is one of the labels of
our self-hosted runners as well.

Avoid this situation by using label names for our self-hosted runner
which are not used by GitHub runners
(https://docs.github.com/en/actions/using-jobs/choosing-the-runner-for-a-job).

See also philips-labs/terraform-aws-github-runner#3290
for a discussion and
https://github.com/philips-labs/terraform-aws-github-runner/blob/6fa667fae7e4302cf643bcdb4ff3c91b1e4ed8d1/lambdas/functions/webhook/src/webhook/index.ts#L83-L86
for the label matching code.

(cherry picked from commit 7b84015)
@imphil
CI: Move Icarus master from experimental to CI
7b49434 
We always want to have the latest release version of a simulator in CI,
move the Icarus 12.0 run from the experimental to the ci bucket.

(cherry picked from commit 6cde941)
@imphil
CI: Run GHDL 4.0 in CI
bba6b34 
Run the latest release version of GHDL in CI (which is version 4.0 at
this time), and move all other GHDL versions into the extended bucket.

(cherry picked from commit 0270a29)
@imphil
CI: Test more Questa and Riviera-PRO versions
0dfa931 
In the previous private CI setup, we ran a number of Questa and
Riviera-PRO versions in extended tests. Add those same tests to our
new CI setup.

(cherry picked from commit 3c767af)
@dependabot @imphil
Bump actions/checkout from 3 to 4
07536fc 
Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
dependabot bot and others added 6 commits April 18, 2024 00:32
@dependabot @imphil
Bump actions/setup-python from 4 to 5
5105da9 
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 4 to 5.
- [Release notes](https://github.com/actions/setup-python/releases)
- [Commits](actions/setup-python@v4...v5)

---
updated-dependencies:
- dependency-name: actions/setup-python
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
(cherry picked from commit cc9694d)
@imphil
CI: Prevent codecov upload failures
aa7728a 
(cherry picked from commit 58b5e38)
@dependabot @imphil
Bump actions/download-artifact from 3 to 4 (cocotb#3592)
49ae436 
Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 3 to 4.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@v3...v4)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
(cherry picked from commit 86b7a17)
@dependabot @imphil
Bump actions/upload-artifact from 3 to 4 (cocotb#3593)
fd91c14 
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 3 to 4.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@v3...v4)

---
updated-dependencies:
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
(cherry picked from commit bc3ddff)
@imphil
CI: Fix artifact upload
7493b4f 
cocotb#3593 updated the version of
actions/upload-artifact from 3 to 4, but didn't perform the migration as
described at
https://github.com/actions/upload-artifact/blob/main/docs/MIGRATION.md.
Do that to fix the broken release CI.

(cherry picked from commit c05526e)
@imphil
CI: Update the codecov GH action to v4
bfd8e95 
Update to the latest version of the codecov GH Action and its uploader.

Most notably, this action now uses nodejs 20 instead of 16, which
GitHub prominently warns about.

Less notably, this action is a significant rewrite from the previous
version, which uses the codecov-cli behind the covers.

(cherry picked from commit e1b92e9)
@imphil imphil requested a review from a team April 17, 2024 22:45
@imphil
Copy link
Member Author

imphil commented Apr 17, 2024

OK, looks promising now. CI should be close to what we have in master, with the notable exceptions of Verilator and NVC (which require code changes to work).

@imphil imphil merged commit 05a5094 into cocotb:stable/1.8 Apr 18, 2024
22 checks passed
@imphil imphil deleted the ci-update-stable-1.8 branch April 18, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktbarrett ktbarrett ktbarrett approved these changes

Assignees
No one assigned
Labels
category:ci continuous integration and unit tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@imphil @ktbarrett @marlonjames