Skip to content

CI: test ./build_emulators.sh #876

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Arielfoever
Copy link
Contributor

Add a build by build_emulators.sh and I believe there's no need to test.

@Arielfoever
Copy link
Contributor Author

See #745

and somehow we need a mirror of gmp.

@Arielfoever
Copy link
Contributor Author

Copy link
Collaborator

@jordancarlin jordancarlin left a comment

Choose a reason for hiding this comment

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

Would be great to get this in, but I don't think this CI job will work until the issue with downloading GMP is addressed (#745 (comment)).

@Arielfoever Arielfoever force-pushed the CIscript branch 2 times, most recently from 5c06d9c to 09af9d5 Compare April 22, 2025 15:06
@jrtc27
Copy link
Collaborator

jrtc27 commented Apr 22, 2025

Building the simulator twice just to test the shell script seems quite wasteful, why not instead adapt the existing build job to use the script?

@jordancarlin
Copy link
Collaborator

That seems reasonable. If we want to make sure that the build works with both static and dynamic gmp, we could also have the build with the shell script happen on a weekly schedule instead of for every PR. Most code changes are unlikely to break it, so that is probably more than sufficient to make sure the script continues working.

@Arielfoever
Copy link
Contributor Author

Would be great to get this in, but I don't think this CI job will work until the issue with downloading GMP is addressed (#745 (comment)).

Fix via #882

@Arielfoever
Copy link
Contributor Author

Building the simulator twice just to test the shell script seems quite wasteful, why not instead adapt the existing build job to use the script?

I would agree with you but I suggest that put these two into repo for some time. When everything's fine we can remove another one.

@Timmmm Timmmm closed this in #882 Apr 24, 2025
@Arielfoever
Copy link
Contributor Author

@Timmmm What is the problem? This is nothing to do with #882. This is CI not mirror.

@jordancarlin
Copy link
Collaborator

I think it was closed automatically by GitHub when #882 was merged because you put "fix #876" in the PR description (which GitHub picks up as a keyword to link PRs/issues). Reopened.

Copy link

github-actions bot commented Apr 27, 2025

Test Results

400 tests  ±0   400 ✅ ±0   2m 40s ⏱️ ±0s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 28da50c. ± Comparison against base commit d9713b2.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

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

LGTM. I think it probably makes sense to make it be another job in compile.yml instead of a new file.

- name: Test simulator
run: ctest --test-dir build --output-junit tests.xml --output-on-failure

- name: Upload test results
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need to upload the test results/event payload since the other workflow will already do this.

Copy link
Collaborator

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

This is duplicating quite a lot - can we just add another matrix combination to the main CI build?

I think something like this could possibly work?

    strategy:
      matrix:
        os: [macos-latest, ubuntu-latest]
        include:
          - os: ubuntu-latest
            variant: build-emulators

@Timmmm
Copy link
Collaborator

Timmmm commented Apr 28, 2025

I think a variant might get complicated. We could potentially use composite actions to add it as a job without duplicating everything though. I've never used them before so I'm not really sure...

@Arielfoever
Copy link
Contributor Author

I think a variant might get complicated. We could potentially use composite actions to add it as a job without duplicating everything though. I've never used them before so I'm not really sure...

I may try but deal with this one and #890 first

@Arielfoever Arielfoever requested review from Timmmm and arichardson May 23, 2025 08:46
@jordancarlin
Copy link
Collaborator

Rather than adding a whole new job, what about updating the matrix in #952 so that one of the CMake versions uses the script for installation? That way, we are getting the benefit of testing the script without adding an ever-growing list of jobs to the CI.

I think it should be possible with something like this in place of the existing "Build and test simulators" step:

      - name: Build simulators
        if: ${{ ! matrix.use_script }}
        run: |
          cmake -S . -B build -GNinja -DCMAKE_BUILD_TYPE=RelWithDebInfo -DFIRST_PARTY_TESTS=TRUE
          ninja -C build all riscv_sim_rv32f riscv_sim_rv64f generated_smt_rv64f generated_docs_rv64d
      - name: Build simulators (with build_simulators.sh)
        if: ${{ matrix.use_script }}
        run: ./build_simulators.sh
      - name: Run Tests
        run: ctest --test-dir build --output-junit tests.xml --output-on-failure

@Arielfoever
Copy link
Contributor Author

Arielfoever commented May 25, 2025

I suggest that this pr should add to tgmm to discuss about the version we want to test. @Timmmm

for instance,

  • different cmake?
  • different system like macos/windows/ubuntu2204/ubuntu2404
  • remove old CI?
  • different CI conditions.

I mentioned that there are a lot of diversions.

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.

5 participants