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

feat(ci): add Lean testing in the CI #871

Open
wants to merge 4 commits into
base: sail2
Choose a base branch
from

Conversation

RaitoBezarius
Copy link

@RaitoBezarius RaitoBezarius commented Jan 13, 2025

This ensures that all changes to the Lean backend are tested by recompiling the Lean files with Lean itself via the coverage CI entrypoint.

I'm not exactly sure if you are fine with the solution to install Lean, I have other options:

  • Bring Nix and install elan via Nix and use the Nix store caching mechanism
  • Remove caching for elan
  • Your preferred solution

Let me know what you prefer and I will adapt this PR. Additionally, I have a Nix scaffolding setup (default/shell) which is quite popular for people working on that environment, if you are interested into me contributing it, please let me know. I can also take maintenance ownership on that if needed, obviously.

cc @javra

Signed-off-by: Raito Bezarius [email protected]

…h EEXIST

If the directory already exist, it's fine, no need to fail with EEXIST.

This fixes the situation where the bug already exist.

Signed-off-by: Raito Bezarius <[email protected]>
@RaitoBezarius
Copy link
Author

I noticed that Lean tests are actually already covered by the test coverage job, I will remove the extension of the core tests and change this to build the emitted Lean project.

/bin/bash is not guaranteed to exist on all Linux distributions, NixOS
does not have one.

Prefer `/usr/bin/env` which should really (empirically) always exist.

Signed-off-by: Raito Bezarius <[email protected]>
Restovers can remains due to spurious failures as we are not catching
exceptions in the middle that may prevent the final `rm` to run.

If we are not doing that, the next run will cause EEXIST errors.

Signed-off-by: Raito Bezarius <[email protected]>
@RaitoBezarius
Copy link
Author

@javra This is ready now, I think.

@RaitoBezarius RaitoBezarius force-pushed the lean-ci branch 3 times, most recently from 5f294bb to e7e5674 Compare January 16, 2025 14:04
@bacam
Copy link
Contributor

bacam commented Jan 16, 2025

Maybe bash -s -- -y?

@bacam
Copy link
Contributor

bacam commented Jan 16, 2025

bash will treat -y as its own option if there's no --.

This reuses the various lean-toolchain information and download an
ad-hoc Lean version based on this.

Signed-off-by: Raito Bezarius <[email protected]>
@RaitoBezarius
Copy link
Author

bash will treat -y as its own option if there's no --.

I was not sure that bash had -- processing, but yeah, I think this should do the job.

Copy link

Test Results

   12 files  ±0     24 suites  ±0   0s ⏱️ ±0s
  748 tests ±0    738 ✅  - 10  0 💤 ±0  0 ❌ ±0  10 🔥 +10 
2 491 runs  ±0  2 481 ✅  - 10  0 💤 ±0  0 ❌ ±0  10 🔥 +10 

For more details on these errors, see this check.

Results for commit 897ed19. ± Comparison against base commit 153c7b6.

@javra
Copy link
Collaborator

javra commented Jan 16, 2025

Besides lake not being found: We'd also need to run the build such that warnings are ignored (because we're using sorry liberally)

uses: actions/cache/restore@v3
with:
path: ~/.elan
key: ${{ matrix.os }}-${{ matrix.version }}-cov
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use the same key here as our opam cache, or will they end up clobbering each other?

Copy link
Author

Choose a reason for hiding this comment

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

I think they are going to clobber each other actually.

Copy link
Author

Choose a reason for hiding this comment

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

(so I would prefix the key by elan-)

@RaitoBezarius
Copy link
Author

Besides lake not being found: We'd also need to run the build such that warnings are ignored (because we're using sorry liberally)

During my local tests, warnings does not cause nonzero exit status and does not make the pipeline fail. But yeah.

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.

4 participants