Detect Python 2 activities before they launch #992
Conversation
|
Thanks. Some interesting ideas there, but change is to wrong place; bin/sugar-launch is not used, activities are started by Sugar in src/jarabe/journal/misc.py launch(), and failure is displayed when LAUNCH_FAILED is detected. Dig deeper and reproduce the problem. |
|
Your opening comment is better in your commit message, see making commits. I'm guessing the quote changes were probably done by a linter, could you review your changes before you make a commit and remove the ones that are just noise? |
|
Changes in the last commit :
Additional changes: Formatting and reverted my own changes to the sugar-launch file. |
|
Haven't reviewed yet, but it seems you've not read my comments above, most of what's in the commit is noise and that makes it difficult to review, please review the changes your linter makes and remove the noise before making a commit.
activity.info never specifies a Python version requirement IIRC, @quozl correct me if I'm wrong. There's no mention of Python 2 in any code, you'd know through the syntax and API changes, so checking for a mention of Python 2 isn't how you'd know what version the code is compatible with. |
There was a problem hiding this comment.
It may be more efficient to change sugar-toolkit-gtk3. I've seen no indication that you've thought of that.
You will need to rewrite the commit messages eventually, but this can wait until you've been through a few more design iterations. Please make sure all new commit messages follow our guidance. Rewrite them in the usual way, e.g. git rebase -i, reword, and git push --force.
@chimosky, yes, there is no python version metadata in the activity.info file.
@amannaik247, you might test Sugar to see what happens when a Python 2 activity is installed and started. Tools such as strace -f would make a trace of the failure, and there may be a single point or exit code that would unambiguously identify the scenario. Have you thought of preventing install or even browsing to the Python 2 app store?
| @@ -93,4 +93,4 @@ if args.debug: | |||
| cmd_args.extend(act_args) | |||
|
|
|||
| os.chdir(str(activity.get_path())) | |||
| os.execvpe(cmd_args[0], cmd_args, activityfactory.get_environment(activity)) | |||
| os.execvpe(cmd_args[0], cmd_args, activityfactory.get_environment(activity)) | |||
There was a problem hiding this comment.
Change removed newline at end of file. Did you review your change before commit?
| @@ -258,20 +259,58 @@ def ready_callback(metadata, source, destination): | |||
| model.copy(metadata, '/', ready_callback=ready_callback) | |||
|
|
|||
|
|
|||
| def is_python2_activity(bundle): | |||
| # Path to the main activity file | |||
| activity_file_path = os.path.join(bundle.get_path(), 'activity.py') | |||
There was a problem hiding this comment.
Many activities do not have an activity.py file.
| if activity_id is None or not activity_id: | ||
| activity_id = activityfactory.create_activity_id() | ||
|
|
||
| logging.debug('launch bundle_id=%s activity_id=%s object_id=%s uri=%s', | ||
| bundle.get_bundle_id(), activity_id, object_id, uri) | ||
|
|
||
| if isinstance(bundle, ContentBundle): | ||
| # Content bundles are a special case: we treat them as launching | ||
| # Browse with a specific URI. |
| @@ -297,7 +336,6 @@ def launch(bundle, activity_id=None, object_id=None, uri=None, color=None, | |||
|
|
|||
| if not shell_model.can_launch_activity_instance(bundle): | |||
| if alert_window is None: | |||
| from jarabe.desktop import homewindow | |||
|
There's no need to apologize, we do appreciate you trying to contribute to Sugar, from everything that's been said above, you could start by actually using Sugar and an activity to know how they work before trying to make a change to them. You could also look at the code in the activity and try to understand what does what, and change things if you have to and test your changes to see if the activity behave as you expected or just to learn how that change affects the activity. We do want you to contribute. Hope this helps! Looking forward to you PR. |
Fixes: #991
The changes I have made:
This implementation ensures that users are informed about potential compatibility issues before attempting to launch an activity that may not work in a Python 3 environment.
Note: I use a black formatter , so some single quotes might have turned into double quotes , creating additional line changes.