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

Testing workflows #164

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Testing workflows #164

wants to merge 1 commit into from

Conversation

mutantcornholio
Copy link

No description provided.

@mutantcornholio mutantcornholio marked this pull request as draft January 15, 2025 17:27
@mutantcornholio mutantcornholio force-pushed the yuri/pr-workflows branch 3 times, most recently from 64abd5b to 804cbaf Compare January 16, 2025 09:37
@mutantcornholio mutantcornholio force-pushed the yuri/pr-workflows branch 13 times, most recently from 27f70dd to 9b757b6 Compare January 21, 2025 13:31
@mutantcornholio mutantcornholio changed the title [WIP]: testing workflows Testing workflows Jan 22, 2025
@mutantcornholio mutantcornholio marked this pull request as ready for review January 22, 2025 08:48
steps:
- uses: actions/checkout@v4

# TODO: move this setup into a reusable docker image;
Copy link
Member

Choose a reason for hiding this comment

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

The tests should run on Windows and MacOS too; instead of a docker image it seems to me we rather make a matrix?

Comment on lines +56 to +64
Command::cargo_bin(common::REVIVE_LLVM)?
.current_dir(test_dir.path())
.arg("build")
.arg("--llvm-projects")
.arg("clang")
.arg("--llvm-projects")
.arg("lld")
.assert()
.success();
Copy link
Member

Choose a reason for hiding this comment

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

Marking it here but can you remove the builtins command below as a drive-by? The builtins are built anyways during the build command.

Comment on lines +105 to +114
Command::cargo_bin(common::REVIVE_LLVM)?
.current_dir(test_dir.path())
.arg("build")
.arg("--llvm-projects")
.arg("clang")
.arg("--llvm-projects")
.arg("lld")
.assert()
.success();

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this and the below command be merged? This here builds LLVM, clang and LLD. Below builds LLVM with tests and coverage.

@@ -116,6 +136,16 @@ fn build_with_sanitizers() -> anyhow::Result<()> {
.assert()
.success();

Command::cargo_bin(common::REVIVE_LLVM)?
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Comment on lines +177 to +178
.arg("--llvm-projects")
.arg("clang")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.arg("--llvm-projects")
.arg("clang")

We should only need LLD; this will make the test consume less resources.


jobs:
test:
runs-on: parity-large
Copy link
Member

Choose a reason for hiding this comment

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

We will release on Windows and MacOS too, so this probably should be a matrix test with those platforms to avoid any problems during releasing.

@mutantcornholio
Copy link
Author

Okay, taking this PR back to draft, let's see how win & mac will run the tests

@mutantcornholio mutantcornholio marked this pull request as draft January 23, 2025 10:09
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.

2 participants