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

Port to Six #383

Closed
wants to merge 10 commits into from
Closed

Port to Six #383

wants to merge 10 commits into from

Conversation

rhl-bthr
Copy link

@rhl-bthr rhl-bthr commented Mar 1, 2018

Changes made/being made in this PR: #382

@rhl-bthr rhl-bthr changed the title [In Progress] Port src/sugar3/graphics to Python3 [In Progress] Port src/sugar3/graphics to six Mar 1, 2018
@rhl-bthr

This comment has been minimized.

@rhl-bthr

This comment has been minimized.

@walterbender
Copy link
Member

It will be hard to test the individual files stand-alone and I don't think our build system tests will be of much use. I will think about it.

Re telepathy, we'll have to do extensive testing of the Python 3 port as well. But maybe we can also ping Collabora to see if they have a Python3 equivalent (more likely to be maintained) toolkit we can begin migrating towards.

@rhl-bthr

This comment has been minimized.

@quozl
Copy link
Contributor

quozl commented Mar 1, 2018

I don't know how to fix the reference to jarabe.model. Can the needed functionality be moved into the toolkit?

Who is pinging Collabora?

I don't know which extensions will be affected.

@quozl
Copy link
Contributor

quozl commented Mar 2, 2018

Reviewed up to 743742d.

@quozl
Copy link
Contributor

quozl commented Mar 3, 2018

Reviewed to 53f6a20.

@rhl-bthr rhl-bthr changed the title [In Progress] Port src/sugar3/graphics to six [In Progress] Port Python code to six Mar 3, 2018
@rhl-bthr

This comment has been minimized.

@quozl
Copy link
Contributor

quozl commented Mar 5, 2018

Reviewed to 89dc9ed.

@quozl quozl mentioned this pull request Mar 9, 2018
10 tasks
@rhl-bthr

This comment has been minimized.

@quozl
Copy link
Contributor

quozl commented Mar 11, 2018

Reviewed to 9699941.

I do not think there will be a port of all activities. It would be nice, but I'm a realist. 😁

Perhaps a better alternative is to use in activity.info;

exec = sugar-activity3 activity.ClassName

Also consider;

  • documentation for exec in src/sugar3/bundle/__init__.py
  • tests/data/sample.activity/activity/activity.info

Now that you have a ported activity, please also contribute to sugarlabs/sugar-docs#145.

@quozl
Copy link
Contributor

quozl commented Mar 26, 2018

Reviewed to a50e843.

@quozl
Copy link
Contributor

quozl commented Apr 1, 2018

Reviewed to 19b551f. These last two deserve to jump the queue and land now, but 19b551f has several merge conflicts. Your ccbf8a4 has been cherry-picked and pushed as 8f047b5.

@quozl
Copy link
Contributor

quozl commented Apr 17, 2018

Reviewed to f833bff.

@quozl
Copy link
Contributor

quozl commented May 15, 2018

Reviewed to 1730e21.

doc/index.rst Outdated
@@ -2,7 +2,7 @@ Sugar Toolkit GTK3
==================

Sugar Toolkit GTK3, or `sugar3`, is a toolkit for writing Sugar
activities in Python:
activities in Python 3:
Copy link
Contributor

Choose a reason for hiding this comment

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

Also in Python 2, so I suggest removing this change.

instance.run_main_loop()

main()
activityinstance.main()
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing new line mark at end of file.


from sugar3.activity import activityinstance

activityinstance.main()
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing new line mark at end of file.

* `exec` - how to execute the activity, e.g. `sugar-activity module.Class`,
* `exec` - how to execute the activity, e.g.
`sugar-activity3 module.Class` (For activities written in Python 3),
`sugar-activity module.Class` (For activities written in Python 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Both lines, should be "activities written for" rather than in. An activity could potentially instantiate both Python 2 and Python 3 interpreters in separate processes.

@quozl
Copy link
Contributor

quozl commented Jun 28, 2018

@Pro-Panda, do you have Debian styled packages for this branch yet?

@@ -15,7 +15,7 @@ GNOME_COMPILE_WARNINGS(maximum)

AC_PATH_PROG([GLIB_GENMARSHAL], [glib-genmarshal])

PYTHON=python2
PYTHON=python3

Choose a reason for hiding this comment

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

It specifically installs for python3
We need toolkit to be 2&3 compatible

Copy link
Author

Choose a reason for hiding this comment

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

yes, I know. I don't yet have a fix for installing for both Python 2 and Python 3.

I am leaving this change as is for now, since LEM doesn't hold here.
Reference: https://en.wikipedia.org/wiki/Law_of_excluded_middle

Copy link
Contributor

Choose a reason for hiding this comment

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

@pro-panda so the sugar toolkit is not yet completely 2 & 3 compatible?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is compatible. You can override PYTHON when you call configure.ac, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes @quozl , as per my undersatnding in that case it will install either python 2 or python 3 and it totally depends upon user how they want to install the toolkit.

@rhl-bthr
Copy link
Author

@quozl, no.
The package should ideally install for both Python 2 and Python 3. I am still working on how to do that

@quozl
Copy link
Contributor

quozl commented Jun 28, 2018

We need the source package (the .tar.xz) to be capable of being installed for both python2 and python3, but not at the same time. The duty to do both is given to the user of the source package; such as the Fedora or Debian packager, or an end-user doing a native install by hand.

So I'll consider the changes for sugar-toolkit-gtk3 to be complete in this respect once PYTHON can be set to either, at the shell prompt. At the moment, we're overriding PYTHON rather than using a default.

@rhl-bthr
Copy link
Author

rhl-bthr commented Jul 8, 2018

@quozl
examples/ticket2855, ticket2999 and ticket3000 don't seem to be linked to https://bugs.sugarlabs.org/ticket/2855, and others respectively. Is there a different ticket they are referring to ?

@quozl
Copy link
Contributor

quozl commented Jul 12, 2018

Is there a different ticket they are referring to ?

Yes, the OLPC bug tracker; e.g. http://dev.laptop.org/ticket/2855

@quozl
Copy link
Contributor

quozl commented Jul 12, 2018

Reviewed 1f34c2d.

I don't understand why 57168c7 is needed; why can't a Sugar Web activity, which is written in JavaScript, use sugar-activity-web ported to Python 3?

@rhl-bthr rhl-bthr changed the title [In Progress] Port Python code to six Port to six Jul 24, 2018
@quozl
Copy link
Contributor

quozl commented Jul 31, 2018

Rebased on master. @Pro-Panda, please save your local branch and check against your GitHub branch.

@quozl
Copy link
Contributor

quozl commented Jul 31, 2018

Reviewed to cc3050f.

@quozl
Copy link
Contributor

quozl commented Aug 3, 2018

Reviewed to bd386f7.

@quozl
Copy link
Contributor

quozl commented Aug 7, 2018

Reviewed to 0ec673f.

@quozl quozl changed the title Port to six Port to Six Aug 13, 2018
@quozl
Copy link
Contributor

quozl commented Sep 24, 2018

@pro-panda, I'm unable to import sugar3 in Python 3. Have you any idea why?

Here's what I've done;

  • installed a Debian Stretch VM, using Sugar Live Build,
  • checked out this branch, autogen.sh, configure, make, make install,
  • tried import sugar3 within Python, which gives ImportError: No module named 'sugar3',

Here's what I've found since;

  • the make install has copied files to /usr/lib/python3.5/site-packages/sugar3,
  • installing python3-serial package has placed files in /usr/lib/python3/dist-packages,
  • renaming site-packages to dist-packages fixes the import fail,
  • moving the sugar3 directory from the first to the second directory fixes the import fail.

So it would seem that our make install uses the wrong place. Looks like it gets this wrong place from /usr/share/aclocal-1.15/python.m4. It seems to be a Debian feature;

@rhl-bthr
Copy link
Author

Thanks for testing. I agree. Reproduced on Ubuntu 16.04.

@ghost
Copy link

ghost commented Sep 29, 2018 via email

@quozl
Copy link
Contributor

quozl commented Oct 27, 2018

I'm guessing from the policy Debian just doesn't support user-installed system-wide Python packages without the user also changing the path.

Best ways forward seem to depend on target;

  • for Sugar Live Build, which does a git clone then make install, we'll have to move the sugar3 directory as a workaround,
  • for Debian or Ubuntu packaging building, no action needed.

I've not evaluated Fedora.

@rhl-bthr
Copy link
Author

for Sugar Live Build, which does a git clone then make install, we'll have to move the sugar3 directory as a workaround,

I agree. I couldn't find an alternate as well

Pro-Panda and others added 10 commits March 13, 2019 12:31
* Embedded the single used instance in itself
* avoid name clashing with the name of the module imported
Activities could not be started, shell.log contained

Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/dbus/connection.py", line 604, in msg_reply_handler
    reply_handler(*message.get_args_list(**get_args_opts))
  File "/usr/lib/python2.7/sugar3/activity/activityfactory.py", line 251, in _activate_reply_handler
    self._create_activity()
  File "/usr/lib/python2.7/sugar3/activity/activityfactory.py", line 222, in _create_activity
    (log_path, log_file) = open_log_file(self._bundle)
  File "/usr/lib/python2.7/sugar3/activity/activityfactory.py", line 140, in open_log_file
    f = open(fd, 'w')
TypeError: coercing to Unicode: need string or buffer, int found
@quozl
Copy link
Contributor

quozl commented Mar 13, 2019

@pro-panda, tested on Ubuntu 18.04 with Python 2 and fixed a problem with starting activities, see d3ac3e5. Is the change rational? I undid your change. See os.fdopen.

@quozl
Copy link
Contributor

quozl commented Mar 13, 2019

Merged as aa8a5e7, thanks!

@quozl quozl closed this Mar 13, 2019
@Aniket21mathur
Copy link
Contributor

Any further work required to be done on this pr? It is still a part of Project Task Checklist for port to python3 project. Thanks!

@quozl
Copy link
Contributor

quozl commented Mar 22, 2019

See #382 for the work remaining to be done.

@Aniket21mathur
Copy link
Contributor

Aniket21mathur commented May 19, 2019

I'm guessing from the policy Debian just doesn't support user-installed system-wide Python packages without the user also changing the path.

Best ways forward seem to depend on target;

  • for Sugar Live Build, which does a git clone then make install, we'll have to move the sugar3 directory as a workaround,
  • for Debian or Ubuntu packaging building, no action needed.

I've not evaluated Fedora.

I think there is a need to include this in the Native build documentation. Suggestions?
I made a pull request sugarlabs/sugar#831 though!

@quozl
Copy link
Contributor

quozl commented May 20, 2019

Sure. It's a mess. Debian and Fedora both seem to have problems dealing with installing Python programs using autotools into /usr/local. It is a path not often tread, so it is unlikely to gain attention.

Sub task make local Fedora and Debian packages for testing by others in each of our Port to Python 3 issues should help to ensure our release, when made, will build and run on Fedora and Debian.

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.

5 participants