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

CI Improvements #4183

Merged
merged 39 commits into from
May 10, 2024
Merged

CI Improvements #4183

merged 39 commits into from
May 10, 2024

Conversation

KrystalDelusion
Copy link
Member

@KrystalDelusion KrystalDelusion commented Feb 5, 2024

Refactor linux/mac tests into unified build and test stages. Builds are performed out-of-tree, compressing and uploading the compiled artifacts to be used in the test stage. The included test suite is run independently from the docs tests. Additional smoke tests for compiler/cpp standard are done separately with a new compile-only target. In all, this should provide a much cleaner idea at a glance of what part of the infrastructure is failing.
Also updates to use latest OS and compiler.

@povik
Copy link
Member

povik commented Feb 5, 2024

I like this change. I saw people get confused by the fact that only some of the "Build and run tests" CI jobs fail, which made it seem as if the test failure was compiler-dependent when in fact it was not. So if this change splits the building and testing into jobs of their own, that will help present the CI results better.

@KrystalDelusion KrystalDelusion force-pushed the krys/refactor-workflows branch 3 times, most recently from a05eb49 to b2ee761 Compare February 6, 2024 05:32
@KrystalDelusion KrystalDelusion marked this pull request as ready for review February 11, 2024 20:20
@mmicko
Copy link
Member

mmicko commented Feb 12, 2024

It would be better if we could reuse compiler testing tasks to also produce artifacts for testing (just for two that needs to be used), looking into that also noticed macOS tests are missing

@KrystalDelusion
Copy link
Member Author

It would be better if we could reuse compiler testing tasks to also produce artifacts for testing (just for two that needs to be used)

I had it setup like that initially (157a94a), but I noticed under Details that it skipped running the tests because the C++20 build was failing, even though the job creating the artifact worked fine. I did some research and it doesn't seem like it allows specifying a specific job from a matrix as a prerequisite, so I added 533fea6 to split the test builds from the compiler test matrix.

looking into that also noticed macOS tests are missing

Added back in now.

@KrystalDelusion KrystalDelusion force-pushed the krys/refactor-workflows branch from 6006bf6 to c91716f Compare March 3, 2024 23:03
@KrystalDelusion KrystalDelusion force-pushed the krys/refactor-workflows branch from 0196557 to 9a44b78 Compare March 24, 2024 20:12
@KrystalDelusion KrystalDelusion requested a review from mmicko as a code owner April 19, 2024 03:47
Copy link
Member

@mmicko mmicko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks great now.

Since those gcc and clang versions do exist for ubuntu-22.04 as well, can we use that one (ubuntu-latest will get promoted to ubuntu-24.04 as it arrives), but 20.04 could be deprecated soon, so if that works please do change it). Maybe just gcc-10 would need to be left on 20.04.

Small nitpick is missing empty line at action.yml at the end.

@KrystalDelusion
Copy link
Member Author

Surprisingly gcc-10 has builds for 24.04, but clang-13 and earlier stop at 22.04 so we will need to update our clang target if we want to use ubuntu-latest for the cpp_std test matrix.

@mmicko mmicko force-pushed the krys/refactor-workflows branch from a6420fa to 38ca47c Compare April 25, 2024 06:40
@mmicko
Copy link
Member

mmicko commented Apr 25, 2024

I have cleaned few things. Main change is that trigger for action should be all branches (non-filtered), VisualStudio action is updated to latest to get rid of warning (NodeJS version) and for regular build/test we always should use latest, so that change is made for ubuntu.
The biggest problem left is, I think, number of jobs that are running. Current state is 18 jobs to do all the builds, and with this PR it rises to 30 jobs. Be aware that when someone make a PR and that is on branch on YosysHQ/yosys it will run 60 jobs and that will take a while. We need to filter this list in compiler tests, since that one is mostly affecting number.
Also I would suggest that for macos (since those are slower and less available machines) we have just compiler test for c++17 in test-compile, since build is already checking c++11

@KrystalDelusion
Copy link
Member Author

Maybe we could split tests so that only the cpp standards run on PRs as a smoke test, and maybe the full range of compilers run periodically (maybe weekly or something)

Separates `test-linux` into `build-linux` and `test-linux`, wherein `build-` builds out of tree, and uploading the build for the `test-` job.
Tar compression is done to retain execution permissions when downloading build artifact.
When calling `make test`, override `TARGETS` and `EXTRA_TARGETS` to prevent rebuild.
e.g. when calling `make clean` out-of-tree
Also reduce `${{}}` expansion in `run` blocks.
Switch build artifact to a default clang build.

Testing with the build artifact locally, `make test` is failing with `/lib/x86_64-linux-gnu/libstdc++.so.6: version 'GLIBCXX_3.4.29' not found`.  Using the gcc-11 build (might be?) installing GLIBCXX_3.4.29 but not linking it into the build.  Rather than trying to get it to link, just use the pre-installed `clang` instead.
Also rename `build-artifact` to use `matrix.os` for compatibility with testing across OS.
`os_name` in include section needs to be explicit (putting it at the end doesn't
apply to the extra jobs).

Move macOS test to extra job instead of doing all gcc/clang (which isn't setup
for mac anyway).

Also adds name to build-yosys task.
Allow test suite to run if, for example, the C++20 builds are failing but C++11 are fine.
Remove compiler and cpp_std from `build-yosys` matrix.  Using `config-$CC` will instead fall back to default values.

Drop `Tool versions` step and introduce `yosys-config` output instead.

Rename `test-builds` to `test-compile`.
`test-macos.yml` included c++17 which was missing in `test-build.yml`.
KrystalDelusion and others added 20 commits May 10, 2024 09:49
New Setup Cpp step uses fully qualified paths for $CC and $CXX so ${CC%%-*} no longer works.
Remove os_name since it's not needed anymore.
Call yosys-config post build extraction for sanity check.
Report absolute path for yosys exe if it can't be found.
Setup script unable to install gcc-12 under ubuntu-20.04.
Get absolute path for `TESTS_DIR` to work from `docs` directory or from `docs/tests` in addition to `yosys` directory.
macOS fails due to missing gvpack, but trying to install graphviz
triggers a Python update which breaks the macOS runner.
Limit compilers to oldest and newest.
Oldest compilers test with minimum supported standard.
Newest compilers test with minimum *and* maximum supported standard.
@KrystalDelusion KrystalDelusion force-pushed the krys/refactor-workflows branch from deb2ce4 to ff730f4 Compare May 9, 2024 21:54
@KrystalDelusion KrystalDelusion changed the title ci: Introduce artifacts CI Improvements May 10, 2024
@mmicko mmicko merged commit 75f01cc into main May 10, 2024
31 checks passed
@mmicko mmicko deleted the krys/refactor-workflows branch May 10, 2024 14:28
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.

3 participants