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

Remove printing of every single log file which could take a very long… #1042

Merged
merged 3 commits into from Feb 20, 2024

Conversation

floyd-fuh
Copy link
Contributor

… time

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Remove printing every leftover file, which takes a significant amount of time

  • What is the current behavior? (You can also link to an open issue here)

EMBA prints every single file, which takes forever, making me quit EMBA

  • What is the new behavior (if this is a feature change)? If possible add a screenshot.

There is no point in printing every single file anyway, if a user wants to know the permissions and files they should be able to execute a command themselves.

It also indirectly solves the problem that this check is done at all during a scan resume. While it is still done (which it shouldn't because obviously there are log files when we resume from those), it is not blocking the entire resume by taking a long time to print file lists.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No

  • Other information:

@m-1-k-3
Copy link
Member

m-1-k-3 commented Feb 19, 2024

The information shown by EMBA is caused because there should no other files in the EMBA directory. Usually you are going to quit EMBA anyways and remove these leftover files.

@m-1-k-3
Copy link
Member

m-1-k-3 commented Feb 19, 2024

Probably only printing the first 100 files or so would be a possible solution.

@floyd-fuh
Copy link
Contributor Author

I think it should be up to the user to decide if he wants to quit and remove the leftovers or not. It's not like the Docker environment is untrusted that runs on my machine... e.g. I think running this entire thing as root is the bigger issue than disclosing a couple of log files to a docker container on the same machine.

Even printing the first 100 files is ridiculously slow, I don't know why.

Also, this serves as a shortcut to also make this issue less of a hassle for the resume feature. Because when using the resume feature to resume from folder ./x it is not very helpful when EMBA warns that there are leftover logs in folder ./x. But that's actually an independent bug.

@m-1-k-3
Copy link
Member

m-1-k-3 commented Feb 19, 2024

I think it should be up to the user to decide if he wants to quit and remove the leftovers or not. It's not like the Docker environment is untrusted that runs on my machine...

It is not because of the docker, it is because you are probably exposing the logs from one firmware test to the other test. From my point it is very useful to get a direct feedback what we have in the EMBA directory. So, we can find the bottle neck, strip it down to more essential output but we will leave it somehow in there.

e.g. I think running this entire thing as root is the bigger issue than disclosing a couple of log files to a docker container on the same machine.

;)

Even printing the first 100 files is ridiculously slow, I don't know why.

probably the print_path is the slow guy in here:

print_path() {

this uses path_attr here
path_attr() {

and here we are doing some find which will probably slow this functionality

Also, this serves as a shortcut to also make this issue less of a hassle for the resume feature. Because when using the resume feature to resume from folder ./x it is not very helpful when EMBA warns that there are leftover logs in folder ./x. But that's actually an independent bug.

You should not let EMBA log to ./x. Use a different log directory outside of the EMBA directory for logging.

@m-1-k-3
Copy link
Member

m-1-k-3 commented Feb 19, 2024

hell this is slow ... I suggest the following two changes :

112:  readarray -t D_LOG_FILES < <( find . \( -path ./external -o -path ./config -o -path ./licenses -o -path ./tools \) -prune -false -o \( -name "*.txt" -o -name "*.log" \) | head -100 )
<snip>
116:    for D_LOG_FILE in "${D_LOG_FILES[@]}" ; do
117:      echo -e "        ""$(orange "${D_LOG_FILE}")"
118:    done

@floyd-fuh
Copy link
Contributor Author

done, also additionally implemented #1043

@m-1-k-3
Copy link
Member

m-1-k-3 commented Feb 20, 2024

Thank you @floyd-fuh for using EMBA and your contribution in making her better! :)

@m-1-k-3 m-1-k-3 merged commit 64c1dd8 into e-m-b-a:master Feb 20, 2024
13 checks passed
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