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

fix: Exit bench init early if Frappe or other apps installed incorrectly #1083

Closed
wants to merge 4 commits into from

Conversation

ceefour
Copy link
Contributor

@ceefour ceefour commented Oct 3, 2020

Fixes #1082

What type of a PR is this?

  • Bug Fix

Please provide enough information so that others can review your pull request:

Detect if command failed, then exit with status 1.

In bench/app.py install_app() -> bench.utils.exec_cmd()


Explain the details for making this change. What existing problem does the pull request solve? The following checklist could help

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Does your commit message have an explanation for your changes and why you'd like us to include them?

Screenshots/GIFs

bench/utils.py Outdated Show resolved Hide resolved
bench/app.py Outdated Show resolved Hide resolved
pip_install_code = exec_cmd("{pip} install {quiet} -U -e {app} {no_cache}".format(
pip=pip_path, quiet=quiet_flag, app=app_path, no_cache=cache_flag))
if pip_install_code:
sys.exit(pip_install_code)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a screenshot of how this would execute now, after this change?

@gavindsouza gavindsouza changed the title refactor: bench init exits if error during "Installing frappe" fix: Exit bench init early if Frappe or other apps installed incorrectly Oct 27, 2020
Co-authored-by: gavin <gavin18d@gmail.com>
@ceefour
Copy link
Contributor Author

ceefour commented Oct 27, 2020

@gavindsouza About screenshot, I documented a lot of the diagnosis process here: https://about.lovia.life/docs/infrastructure/erpnext/install-erpnext-on-ubuntu-vm/#Troubleshooting_3_psycopg2

So this was what happened:

image

My patch stopped the execution if an error happened, since it doesn't make sense to continue....... which will throw another error anyway (with even more confusing error).

By stopping early, it's easier for user to figure out what went wrong, fix it, then they can retry.

@gavindsouza
Copy link
Collaborator

gavindsouza commented Oct 28, 2020

@ceefour I get that part. I wasn't able to replicate failing dependency setups and I'd like to see how bench would exit with the proposed fix. Can you show me a replicable screenshot so that there are no unknowns before we go ahead with this change?

Also, there's an indentation error. Can you fix that?

@ceefour
Copy link
Contributor Author

ceefour commented Oct 28, 2020

One way that happened to me is low disk space..

So to reproduce it:

  1. Do not install any apps yet
  2. Make it so the disk space is left only a bit (probably easier to do inside a Docker container)
  3. Do bench get-app erpnext.. When it fails installing frappe, you'll see it continue etc.
  4. Compare behavior of above but with patch applied

@@ -174,7 +174,13 @@ def install_app(app, bench_path=".", verbose=False, no_cache=False, restart_benc
app_path = os.path.join(bench_path, "apps", app)
cache_flag = "--no-cache-dir" if no_cache else ""

exec_cmd("{pip} install {quiet} -U -e {app} {no_cache}".format(pip=pip_path, quiet=quiet_flag, app=app_path, no_cache=cache_flag))
pip_install_code = exec_cmd(
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ceefour Indentation error

@gavindsouza
Copy link
Collaborator

Now, bench exits whenever any exec_cmd fails via gavindsouza@3995b92. That should fix the issue this PR was addressing.

@ceefour ceefour deleted the patch-install-app branch December 1, 2021 02:09
@ceefour
Copy link
Contributor Author

ceefour commented Dec 1, 2021

Thank youuu @gavindsouza

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.

bench init should exit with status 1 on error during "Installing frappe"
2 participants