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

Run_script echo output to console, and capture errors from script #3277

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alextrical
Copy link

Description

run_script steps are now run with output echoed into the terminal, allowing the user to see the status of a script.
Errors raised from the script are now have handling.

Any Raised errors other than 0, will create an error message but continue with the rest of the manifest (non blocking)
However it could be advisable to gracefully terminate the application installation with a GUI message?

Fixes #3275

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.
Provide instructions so we can reproduce.

  • Run any application manifest with a "run_script" step. The output will be echoed into the terminal.

capture_output=True,
)
logging.info(f"Output: \n{result.stdout.decode()}\n")
if result.returncode != 0:
Copy link
Member

@mirkobrombin mirkobrombin Feb 14, 2024

Choose a reason for hiding this comment

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

change this to return result.returncode == 0 this will return True if the condition meet and False otherwise. If you look at how the install function executes the installer' steps, it checks if one of them returns False

if not self.__perform_steps(_config, steps):
return Result(
False, data={"message": "Installer is not well configured."}
)
and then return a proper Result object according to that.

Currently, there is no easy way to show a message to the user graphically. It could be possible by making each step returning a tuple (boolean, message) and have the install function append that message to the Result object, then updating the installer view in the frontend to use that message if available

def __install(self, *_args):
self.set_deletable(False)
self.stack.set_visible_child_name("page_install")
@GtkUtils.run_in_main_loop
def set_status(result, error=False):
if result.ok:
return self.__installed()
_err = result.data.get("message", _("Installer failed with unknown error"))
self.__error(_err)
self.set_steps(self.manager.installer_manager.count_steps(self.installer))
RunAsync(
task_func=self.manager.installer_manager.install,
callback=set_status,
config=self.config,
installer=self.installer,
step_fn=self.next_step,
local_resources=self.__final_resources,
)

Copy link
Author

Choose a reason for hiding this comment

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

adding the line return result.returncode == 0 results in a known failure (exit 1) in the script not even putting a warning in the Console, and just continuing with the rest of the manifest steps.

Same goes for adding
return False
or

      return Result( 
         False, data={"message": "Installer is not well configured."} 
     ) 

Bottles just continues on with the rest of the manifest regardless of the failure

It looks like there is something missing to handle the return at this point, as it continues regardless of any state returned

@mirkobrombin
Copy link
Member

It's handled here

it will simply never complete without an ok result. Can' t take a look in a short time.

@alextrical
Copy link
Author

alextrical commented Feb 14, 2024

It seems like I'm missing a couple of tricks today, but as far as i can tell it always completes regardless of the result being set to Null, True or False. I can't get it to fail the install process from a script by setting the "Result"

Edit: Welp it seems i accidentally closed this... For now I should probably take a break from projects for a few days while i get other things sorted out

@alextrical alextrical closed this Feb 14, 2024
@mirkobrombin
Copy link
Member

Let me know if I should re-open it

@alextrical
Copy link
Author

If you could, that would be great.

I think the next step is working out why the Result doesn't look like it's being checked. As either False or no Result being set don't stop the manifest proceeding to the next step

@mirkobrombin mirkobrombin reopened this Feb 19, 2024
Copy link

fab-sonarqube bot commented Feb 19, 2024

Copy link
Contributor

Pylint result on modfied files:
************* Module bottles.backend.managers.installer
bottles/backend/managers/installer.py:100:20: I1101: Module 'pycurl' has no 'Curl' member, but source is unavailable. Consider adding this module to extension-pkg-allow-list if you want to perform analysis based on run-time introspection of living objects. (c-extension-no-member)
bottles/backend/managers/installer.py:102:38: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
bottles/backend/managers/installer.py:252:17: W1510: 'subprocess.run' used without explicitly defining the value for 'check'. (subprocess-run-check)
bottles/backend/managers/installer.py:231:4: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)
bottles/backend/managers/installer.py:361:4: R0913: Too many arguments (6/5) (too-many-arguments)

@mirkobrombin mirkobrombin marked this pull request as draft March 25, 2024 20:37
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.

[Bug]: Run_script not working
2 participants