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

Cleanup setup scripts #627

Merged
merged 4 commits into from
Sep 7, 2022
Merged

Cleanup setup scripts #627

merged 4 commits into from
Sep 7, 2022

Conversation

angelhof
Copy link
Member

@angelhof angelhof commented Sep 6, 2022

Clean up the setup scripts by removing unnecessary components and by preserving all output for better debugability.

Signed-off-by: Konstantinos Kallas <konstantinos.kallas@hotmail.com>
Signed-off-by: Konstantinos Kallas <konstantinos.kallas@hotmail.com>
Signed-off-by: Konstantinos Kallas <konstantinos.kallas@hotmail.com>
@angelhof
Copy link
Member Author

angelhof commented Sep 6, 2022

@nvasilakis @mgree the setup script no longer sends output to log files to improve debugability (especially when installing PaSh in CI and in docker images). I think this is fine in general too. If you think that is too verbose, we could hide output by default and only show it when a -v flag is used. However, I think this is unnecessary for now, and we can just proceed with this PR. Let me know what you think.

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

OS:ubuntu-20.04
Tue Sep 6 17:59:11 UTC 2022
intro: 2/2 tests passed.
interface: 39/39 tests passed.
compiler: 54/54 tests passed.
agg: 109/109 tests passed.

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

OS = Debian 10
CPU = Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz
Ram = 15752
Hash = 56d8b4b
Kernel= Linux 4.15.0-167-generic x86_64

benchmark tests passed failed untested unresolved unsupported not_in_use other_status
posix 494 375 41 31 6 40 1 0
intro 2 2 0 0 0 0 0 0
interface 39 39 0 0 0 0 0 0
compiler 54 54 0 0 0 0 0 0
aggregator 109 108 1 0 0 0 0 0

@mgree
Copy link
Collaborator

mgree commented Sep 6, 2022

Looks good to me---I think no need for the flag or anything. It might make sense to include some meta commands to group stuff, as in https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#grouping-log-lines. But definitely not a big deal.

It may also make sense to test for CI (or some other relevant variable, per https://docs.github.com/en/actions/learn-github-actions/environment-variables) to determine whether or not to hide output by default.

Copy link
Collaborator

@mgree mgree 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 can imagine adding

[ "$CI" != "true" ] && exec 3>&1 4>&2 >FOO.log 2>&1

at the top of scripts to hide output by default (saving stdout/stderr in fd 3/4 if we need them later for explicit, deliberate, always-on output)

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

OS:ubuntu-20.04
Tue Sep 6 19:01:34 UTC 2022
intro: 2/2 tests passed.
interface: 39/39 tests passed.
compiler: 54/54 tests passed.
agg: 109/109 tests passed.

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

OS = Debian 10
CPU = Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz
Ram = 15752
Hash = 4e45775
Kernel= Linux 4.15.0-167-generic x86_64

benchmark tests passed failed untested unresolved unsupported not_in_use other_status
posix 494 375 41 31 6 40 1 0
intro 2 2 0 0 0 0 0 0
interface 39 39 0 0 0 0 0 0
compiler 54 54 0 0 0 0 0 0
aggregator 109 108 1 0 0 0 0 0

@angelhof
Copy link
Member Author

angelhof commented Sep 6, 2022

lgtm; i can imagine adding

[ "$CI" != "true" ] && exec 3>&1 4>&2 >FOO.log 2>&1

at the top of scripts to hide output by default (saving stdout/stderr in fd 3/4 if we need them later for explicit, deliberate, always-on output)

@mgree The problem with this is that it hides all output, right? Would it be better if we instead made a function that can wrap all commands that produce heavy output and hides their output if an environment variable is set, for example:

hide_if_not_verbose()
{
    if [ "$verbose" == "true" ] || [ "$CI" == "true" ]; then
        "$@"
    else
        >>log.txt 2>&1 "$@"
    fi
}

which could be called as follows:

hide_if_not_verbose python3 -m pip install ...

and we can have big progress output always on.

@mgree
Copy link
Collaborator

mgree commented Sep 7, 2022

This seems reasonable to me. Maybe better to only check verbose in the command and default verbose when CI is set?

@angelhof angelhof merged commit ffa074f into main Sep 7, 2022
@angelhof angelhof deleted the cleanup-setup-scripts branch September 7, 2022 16:14
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.

None yet

2 participants