Merge changes in to support parsing bash scripts #737
Conversation
BolunThompson
force-pushed
the
bash-merge
branch
from
1ae61d4 to
d451f5a
Compare
Signed-off-by: Bolun Thompson <[email protected]>
Signed-off-by: Bolun Thompson <[email protected]>
Signed-off-by: Bolun Thompson <[email protected]>
Signed-off-by: Bolun Thompson <[email protected]>
The bash tests contain scripts which use UTF-8 only characters, but, by default, Python throws an exception when writing non-ASCII characters to a file. Signed-off-by: Bolun Thompson <[email protected]>
Signed-off-by: Bolun Thompson <[email protected]>
Signed-off-by: Bolun Thompson <[email protected]>
Signed-off-by: Bolun Thompson <[email protected]>
BolunThompson
force-pushed
the
bash-merge
branch
from
d451f5a to
b550e15
Compare
|
I merged the depending PRs, but I suspect we also need to create a PyPI release of libbash correct? |
|
You’re correct — thanks for catching that. PR for it is binpash/libbash#3. |
Signed-off-by: Bolun Thompson <[email protected]>
|
Great, we also need to push this release to PyPI, for which I will wait for Seth to give me access to the libbash PyPI repo. |
|
After merging libbash and pushing the repo to PyPI we should be able to continue working on this :) |
|
OS = |
|
OS:ubuntu-20.04 |
|
OS = |
|
OS:ubuntu-20.04 |
There was a problem hiding this comment.
Great job @BolunThompson and @sethsabar !! The only change that we need to do is to make sure that bash tests run in CI (not by modifying the yml file but rather the test scripts). In my opinion, it would be better to just modify all test files to run both with and without --bash for all tests (avoiding the control flow that exists currently), however I don't have a strong opinion on that.
| @@ -179,6 +192,9 @@ execute_tests() { | |||
| export pash_output="${intermediary_dir}/${microbenchmark}_${n_in}_pash_output" | |||
| export script_conf=${microbenchmark}_${n_in} | |||
| echo '' > "${pash_time}" | |||
| if [ "$test_mode" == "bash" ]; then | |||
| @@ -96,12 +100,21 @@ pipeline_microbenchmarks=( | |||
| execute_pash_and_check_diff() { | |||
| TIMEFORMAT="%3R" # %3U %3S" | |||
| if [ "$DEBUG" -eq 1 ]; then | |||
| { time "$PASH_TOP/pa.sh" $@ ; } 1> "$pash_output" 2> >(tee -a "${pash_time}" >&2) && | |||
| diff -s "$seq_output" "$pash_output" | head | tee -a "${pash_time}" >&2 | |||
| if [ "$test_mode" == "bash" ]; then | |||
There was a problem hiding this comment.
I feel that we can just modify the file to run tests both with --bash and without (modifying the configurations array above).
scripts/run_tests_bash.sh
Outdated
| @@ -0,0 +1,24 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| set -x e | |||
There was a problem hiding this comment.
We should run this script in CI!
| @@ -328,7 +339,7 @@ test_IFS() | |||
| } | |||
|
|
|||
| ## We run all tests composed with && to exit on the first that fails | |||
| if [ "$#" -eq 0 ]; then | |||
| if [ "$#" -eq 0 ] || [ "$test_mode" = "bash" ]; then | |||
There was a problem hiding this comment.
I would really remove all these control flow checks and make sure that all tests always run both with and without --bash.
Also changes it so that only the bash tests only run in bash mode, which I feel is fair since they test bash only features
|
OS = |
|
OS = |
|
OS:ubuntu-20.04 |
|
OS:ubuntu-20.04 |
|
OS = |
|
OS:ubuntu-20.04 |
|
The current problem is that pash expands words in bash mode by calling I’m finishing |
Ohh, that is a bummer... Happy to discuss the solution if needed! |
Code written by @sethsabar. The tests pass with the changes from binpash/shasta#5 and binpash/libbash#1 (CI will fail until those are merged in).