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

test: Use the bash container and a matrix strategy for bash versions #616

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
+40 −4
Open

test: Use the bash container and a matrix strategy for bash versions #616

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
681f897
test: Use the bash container and a matrix strategy for bash versions
lens0021 Nov 24, 2024
751e802
Add one more matrix for sed versions
lens0021 Nov 24, 2024
9ea18d8
Run as non-root user
lens0021 Nov 24, 2024
a306e38
Update rust.yml: --network host
lens0021 Nov 25, 2024
60b8e7b
Fix the syntax of the workflow
lens0021 Nov 25, 2024
78c8e02
Update rust.yml
lens0021 Nov 25, 2024
942c704
Update rust.yml
lens0021 Nov 25, 2024
a0ab353
Update rust.yml
lens0021 Nov 25, 2024
4488be2
Fix bash syntax
lens0021 Nov 25, 2024
325ddda
Bash 5.3 is alpha
lens0021 Nov 25, 2024
a35b1b6
Remove unneeded `return` statement
lens0021 Nov 25, 2024
fe36bfe
Remove a debugging step
lens0021 Nov 25, 2024
361bf9b
Exclude BusyBox
lens0021 Nov 26, 2024
a6e56e1
Edit besh_versions
lens0021 Nov 26, 2024
4454680
Rename the name of the test container
lens0021 Nov 26, 2024
595ca53
create docker command from PATH
lens0021 Nov 26, 2024
d78e06d
Use readable matrix names
lens0021 Nov 26, 2024
f564718
Disable failed bash 3.1 and bash 3.0 tests
lens0021 Nov 26, 2024
4255110
re-order matrix
lens0021 Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,27 @@ jobs:
build_and_test:
name: Rust and clippy tests
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
sed:
- GNU
bash_version:
- "5.2"
- "5.1"
- "5.0"
- "4.4"
- "4.3"
- "4.2"
- "4.1"
- "4.0"
- "3.2"
- "3.1"
- "3.0"
lens0021 marked this conversation as resolved.
Show resolved Hide resolved
# Uncomment the next matrix when https://github.com/amber-lang/amber/issues/617 is resolved
# include:
# - sed: BusyBox
# bash_version: "latest"
steps:
- uses: actions/checkout@v4
- uses: awalsh128/cache-apt-pkgs-action@latest
Expand All @@ -53,6 +74,16 @@ jobs:
# the binary will be used by the next cargo test step
run: cargo build
- name: Run cargo tests
run: cargo test --all-targets --all-features
env:
GITHUB_ACTIONS_BASH_CONTAINER: 1
run: |
docker run --volume $PWD:/root --network host --detach --name test_container bash:${{ matrix.bash_version }} sleep infinity
# coreutils includes mktemp
docker exec test_container apk add coreutils curl
if [ "${{ matrix.sed }}" == "GNU" ]; then
docker exec test_container apk add sed
fi
cargo test --all-targets --all-features
lens0021 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

i understand that you have created a docker container to run tests inside it. this command is run at the host machine, therefore rendering the container useless

docker rm --force test_container
- name: Run clippy check
run: cargo clippy --all-targets --all-features -- -D warnings
lens0021 marked this conversation as resolved.
Show resolved Hide resolved
12 changes: 9 additions & 3 deletions src/compiler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,14 @@ impl AmberCompiler {

#[cfg(not(windows))]
fn find_bash() -> Option<Command> {
let mut command = Command::new("/usr/bin/env");
command.arg("bash");
Some(command)
if env::var("GITHUB_ACTIONS_BASH_CONTAINER").is_ok() {
Copy link
Member

Choose a reason for hiding this comment

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

@hdwalters it isn't better that this code there is only on a debug build like you did for another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like debug and release builds having different functionality. I reverted this behaviour in your documentation usage code when I did the revamped CLI; it's now enabled with a new command line option amber docs --usage.

Copy link
Contributor

@hdwalters hdwalters Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Although in this case, it may make sense. I suggest asking for opinions on the "general" Discord group, and flagging @lens0021.

Copy link
Contributor Author

@lens0021 lens0021 Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It will be helpful if there is a CLI option to choose other bash executable or kind of entry point. Some other environment, for example Termux(an Android terminal emulator) has no /usr/bin/env so always fails to run amber eval. (amber build is ok)

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, but that's out of scope for this PR. AIUI, @Mte90's point was that since your GITHUB_ACTIONS_BASH_CONTAINER change was intended to support cargo test in a Docker container, it should probably not be enabled for release builds. If we agree on this, we could do something like:

#[cfg(not(windows))]
#[cfg(debug)]
fn find_bash() -> Option<Command> {
    // Use docker or /usr/bin/env...
}

#[cfg(not(windows))]
#[cfg(not(debug))]
fn find_bash() -> Option<Command> {
    // Use /usr/bin/env...
}

Copy link
Member

Choose a reason for hiding this comment

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

Yep that's my idea.

let mut command = Command::new("docker");
command.args(["exec", "--workdir", "/root", "--user", "405", "test_container", "bash"]);
Some(command)
} else {
let mut command = Command::new("/usr/bin/env");
command.arg("bash");
Some(command)
}
}
}
Loading