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 #382

Open
quozl opened this issue Feb 26, 2018 · 20 comments

Comments

4 participants
@quozl
Copy link
Contributor

commented Feb 26, 2018

Port the Sugar Toolkit for GTK+ 3 to Six, providing both Python 2 and Python 3 bindings from this package.

Progress goals:

  • fix a critical accidental dependency; remove the from jarabe.model import shell from src/sugar3/graphics/popwindow.py and use some other design,
  • port C code in src/sugar3 none of the C files include Python,h,
  • port toolkit code in src/sugar3 which calls C code, and test that the C code can be called from REPL,
  • port toolkit code in src/sugar3/graphics,
  • port graphics examples, and test that examples work,
  • port toolkit code in src/sugar3/activity,
  • port the Hello World activity,
  • port all remaining toolkit code, such as src/sugar3/datastore, presence, bundle,
  • port to TelepathyGLib,
  • port make files, and test that "make install" installs both bindings,
  • regenerate documentation,
  • make local Fedora and Debian packages for testing by others.
  • release new version of toolkit.

this is an issue for tracking a major project which is larger than one person's contributions, so developers should make pull requests to satisfy any of these items, and not ask to own the issue.

@quozl

This comment was marked as off-topic.

Copy link
Contributor Author

commented Mar 9, 2018

A discussion began in pull-request #385 about redundant work. As your release manager, my opinion is;

  • everyone who contributes to issue #382 should cherry-pick each other's patches into a local branch before starting any work, so that you can avoid doing work that was already done.
  • you should all learn how to and be comfortable enough to maintain several branches within your own local clones.
  • when making a pull request, rebase your changes to master.
  • a pull request may contain work by you alone, or it may contain patches by others too.
  • a pull request may be for review only, or for review and merging.
  • to aid in review and collaboration, you may use a branch name of python3.

I'm unwilling to merge any pull requests until the whole issue is resolved; as I don't want master branch in a broken state in any point of git history, because this breaks use of "git bisect".

@quozl quozl referenced this issue Mar 11, 2018

Open

Port to Python 3 #2

@quozl quozl referenced this issue Apr 12, 2018

Closed

Port to Python 3 #24

@quozl

This comment was marked as resolved.

Copy link
Contributor Author

commented May 1, 2018

Reviewed the critical dependency during a meeting. It is probably a coding error; if PopWindow is used by an activity, the reference counted variable would not be shared with the shell, because the shell is in a different process. The shell is the only consumer of PopWindow. No activities use PopWindow. So the code can be removed from the toolkit and reimplemented in src/jarabe/desktop/activitychooser.py ActivityChooser.

@Aniket21mathur

This comment was marked as outdated.

Copy link
Member

commented May 25, 2019

[ ] port the Hello World activity,

Ported by @pro-panda .

@Aniket21mathur

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

* make local Fedora and Debian packages for testing by others.

Link to Debian packages (Python 2 version).
https://github.com/Aniket21mathur/sugar-toolkit-gtk3-packages

@Aniket21mathur

This comment was marked as outdated.

Copy link
Member

commented Jun 29, 2019

* port toolkit code in `src/sugar3` which calls C code, and test that the C code can be called from REPL,

@pro-panda @quozl I have not found any .so files in the respective directory. Also did a manual search, no import of any .c file in a python file.

@Aniket21mathur

This comment was marked as resolved.

Copy link
Member

commented Jun 29, 2019

* regenerate documentation,

I wanted to know which documentation is referred here, and how do we regenerate it?

@Aniket21mathur

This comment was marked as resolved.

Copy link
Member

commented Jun 29, 2019

* port graphics examples, and test that examples work,

Already ported by @pro-panda. I reviewed the changes again. :-)

@pro-panda

This comment was marked as resolved.

Copy link
Member

commented Jun 30, 2019

* regenerate documentation,

I wanted to know which documentation is referred here, and how do we regenerate it?

We generate documentation using Sphinx. It is currently hosted at https://developer.sugarlabs.org/sugar3/

You can use https://github.com/sugarlabs/sugar-toolkit-gtk3/blob/master/make-doc.sh to generate documentation.
It is generated from docstrings like this and this.
I briefly reviewed aa8a5e7 and did not see any changes in the docs. I've not reviewed this in other commits yet.

@pro-panda

This comment was marked as resolved.

Copy link
Member

commented Jun 30, 2019

* port toolkit code in `src/sugar3` which calls C code, and test that the C code can be called from REPL,

@pro-panda @quozl I have not found any .so files in the respective directory. Also did a manual search, no import of any .c file in a python file.

Thanks, python does not support importing .c files directly. You must compile them and generate a .so to use it. We had one case in sugar-datastore.

I agree, there are no such cases in sugar-toolkit

@pro-panda

This comment was marked as resolved.

Copy link
Member

commented Jun 30, 2019

* port graphics examples, and test that examples work,

Already ported by @pro-panda. I reviewed the changes again. :-)

Thanks

@Aniket21mathur

This comment was marked as resolved.

Copy link
Member

commented Jul 1, 2019

I briefly reviewed aa8a5e7 and did not see any changes in the docs.

Thanks Agreed.
I also looked for changes in https://github.com/sugarlabs/sugar-toolkit-gtk3/tree/master/doc, didn't find any, anywhere else to look?

@pro-panda

This comment was marked as resolved.

Copy link
Member

commented Jul 2, 2019

I also looked for changes in https://github.com/sugarlabs/sugar-toolkit-gtk3/tree/master/doc, didn't find any, anywhere else to look?

Sorry, what did you look for ?
You can go through each commit relating to the port to Python 3, which has been or will be pushed to master, and see if the changes made require changing the docs as well. If yes, please do.

Rephrasing my previous statement,

I briefly reviewed aa8a5e7 and did not see any changes in the docs.

I briefly reviewed aa8a5e7 and did not see any instances where the docs need to be updated

@quozl

This comment was marked as resolved.

Copy link
Contributor Author

commented Jul 2, 2019

@Aniket21mathur

This comment was marked as resolved.

Copy link
Member

commented Jul 3, 2019

@quozl @pro-panda thanks for explaining. Comapred changes in https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/412/files with sugar3.activity.activity module , sugar3.presence.activity module, sugar3.presence.buddy module, sugar3.presence.connectionmanager module, sugar3.presence.presenceservice module, sugar3.presence.sugartubeconn module, sugar3.presence.tubeconn module API docs. The telepathy port does not include any changes in function names mentioned in the API docs, nor does it contradicts their description. I see no edits to the current documentation, but their might be something that we can add, like how to use telepathy_text_chan in activities? Need suggestions.

@quozl

This comment was marked as resolved.

Copy link
Contributor Author

commented Jul 6, 2019

Reviewed changes so far; we've completed documentation changes; of the changes to API that were made, the documentation did not originally describe what was changed. Tracking the opportunity in another issue.

@sukhdeepg

This comment was marked as resolved.

Copy link

commented Jul 18, 2019

@Aniket21mathur as you quoted:

As the toolkit supports both Python and Python 3 we should have multi-version packages for the toolkit

I had a question:
I checked the python code in the repository, for the py files I saw, it was python3. So, how are we providing python2 support? and to completely understand the package meaning, is it what we install using apt(in case of Ubuntu) or the zip file which is created after make dist. Thanks in advance.

@pro-panda

This comment was marked as resolved.

Copy link
Member

commented Jul 18, 2019

Hi @sukhdeepg,

this is not the best place to ask doubts. Please use the mailing list.

I checked the python code in the repository, for the py files I saw, it was python3

Which files did you see ?
A .py file does not enforce the Python version with which it is supposed to be executed. I'm assuming you confused it with shebang line.

...and to completely understand the package meaning, is it what we install using apt(in case of Ubuntu) or the zip file which is created after make dist

Yes, you usually install packages using apt, and use the make dist command in the process of generating commands. I've skipped a lot of details on how these work. Feel free to explore

@quozl

This comment was marked as resolved.

Copy link
Contributor Author

commented Jul 18, 2019

Component sugar-toolkit-gtk3 branch master is both Python 2 and Python 3 compatible right now. This was achieved using Six. Both autogen.sh and configure accept an argument to select the version of Python to build for.

Hiding subthread as resolved. Better forum is mailing list. But I don't think you had exhausted all sources of information before asking your question. 😁

@sukhdeepg

This comment was marked as off-topic.

Copy link

commented Jul 19, 2019

@pro-panda @quozl my bad for not researching thoroughly. Point taken and understood 🙂

@quozl

This comment was marked as off-topic.

Copy link
Contributor Author

commented Jul 19, 2019

No worries. I saw you subscribed to the mailing list. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.