search

sugarlabs / sugar-toolkit-gtk3

Sugar Learning Environment, Activity Toolkit, GTK 3.
GNU Lesser General Public License v2.1
21 stars 80 forks source link

Add setup.py --destdir support #454

Closed quozl closed 3 years ago

quozl commented 3 years ago
chimosky commented 3 years ago

Tried testing but I don't notice any changes, testing currently without supplying --prefix to master installs to /usr/share/sugar/activities/activityname.activity/ and same also happens for this branch.

How did you test?

quozl commented 3 years ago

Thanks for testing. That's expected. My test method was;

e.g.

python3 setup.py install
python3 setup.py install --prefix /usr/local
python3 setup.py install --prefix /usr --destdir /tmp/target

Expected output;

--prefix --destdir destinations
omitted omitted /usr/share/locale/*/LC_MESSAGES/org.laptop.Calculate.mo
/usr/share/sugar/activities/Calculate.activity
/usr/share/applications/org.laptop.Calculate.activity.desktop
/usr/share/metainfo/org.laptop.Calculate.appdata.xml
/usr/local omitted /usr/local/share/sugar/activities/Calculate.activity/locale/hy/activity.linfo
/usr/local/share/locale/hy/LC_MESSAGES/org.laptop.Calculate.mo
/usr/local/share/applications/org.laptop.Calculate.activity.desktop
/usr/local/share/metainfo/org.laptop.Calculate.appdata.xml
/usr /tmp/prefix /tmp/target/usr/share/locale/*/LC_MESSAGES/org.laptop.Calculate.mo
/tmp/target/usr/share/sugar/activities/Calculate.activity
/tmp/target/usr/share/applications/org.laptop.Calculate.activity.desktop
/tmp/target/usr/share/metainfo/org.laptop.Calculate.appdata.xml
chimosky commented 3 years ago

Testing on live build and when I test with --prefix set to /usr/local the activity files get installed to /usr/local and trying to install with --destdir throws an error ./setup.py: error: unrecognized arguments: --destdir /tmp/target and when the activity is installed with --prefix set to /usr/local, the desktop file doesn't exist in /usr/share/applications/.

I haven't figured why --destdir throws an error as I built from is453 and verified the changes were applied in /usr/lib/python3.7/site-packages/sugar3/activity/bundlebuilder.py.

After reinstall, the build path is still embedded in the desktop file.

Icon = /build/sugar-calculate-activity-44/debian/sugar-calculate-activity//usr/share/sugar/activities/Calculate.activity/activity/calculate.svg
Path = /build/sugar-calculate-activity-44/debian/sugar-calculate-activity//usr/share/sugar/activities/Calculate.activity

The Exec seems odd; Exec = sugar-activity calculate.Calculate -s, it definitely shouldn't be sugar-activity.

quozl commented 3 years ago

An unrecognised argument for --destdir means my patch isn't applied to the bundle builder being used. There's more than one;

The exec shows the source you are using is for the Sugar Toolkit for GTK 3 on Python 2, and you should be able to confirm that by reading the first line of setup.py. Where did the source code come from? If it is a git repository, what's the git hash?

quozl commented 3 years ago

Downside of 8250c17 and f9b2b6c is withdrawal of Python 2 support by the toolkit. Comments?

walterbender commented 3 years ago

I think it is time. It is enough work to support the Python 3 code given our limited resources.

quozl commented 3 years ago

It will stop further porting to Python 3. That's also a good idea, because it will ensure our smaller number of activities will better match our limited resources.

srevinsaju commented 3 years ago

So, does this mean, we stop migrating to python3?

quozl commented 3 years ago

By withdrawing the toolkit for Python 2, we reduce availability of the environment used for porting, such as Sugar Live Build or Ubuntu. Porting could still occur, but is made more difficult because the environment used for porting would be ageing rapidly.

Putting it differently, are your changes in 8250c17 and f9b2b6c sufficient enough to justify this impact. I suspect not. So I'm looking for further input.

srevinsaju commented 3 years ago

By withdrawing the toolkit for Python 2, we reduce availability of the environment used for porting, such as Sugar Live Build or Ubuntu. Porting could still occur, but is made more difficult because the environment used for porting would be ageing rapidly.

Putting it differently, are your changes in 8250c17 and f9b2b6c sufficient enough to justify this impact. I suspect not. So I'm looking for further input.

It is quite complex for me to understand. Why do we stop migrating to Python 3? Are we EOL'ing Sugar? This would mean lesser outreach, distribution I suppose. I did not get the objective :(

quozl commented 3 years ago

Happy to explain again.

Porting an activity from Python 2 to Python 3 requires both environments to be working, otherwise the developer can't tell if program behaviours they observe are correctly implemented. We had one or two people try to port without having a Python 2 environment available, and it has been a terrible outcome, because they introduce bugs, regressions, and unexpected features. They end up relying on other people to notice these, because without a Python 2 environment they can't see the difference.

My objective had been to support both Python 2 and Python 3 in the Sugar Toolkit, see https://github.com/sugarlabs/sugar-toolkit-gtk3/issues/382. Your objective in 8250c17 is to increase performance by removing a check that is now available in Python 3, but the code you've changed fails in Python 2.

So we have conflicting objectives, and I'm asking for developers to decide which objective takes precedence.

srevinsaju commented 3 years ago

Happy to explain again.

Porting an activity from Python 2 to Python 3 requires both environments to be working, otherwise the developer can't tell if program behaviours they observe are correctly implemented. We had one or two people try to port without having a Python 2 environment available, and it has been a terrible outcome, because they introduce bugs, regressions, and unexpected features. They end up relying on other people to notice these, because without a Python 2 environment they can't see the difference.

My objective had been to support both Python 2 and Python 3 in the Sugar Toolkit, see #382. Your objective in 8250c17 is to increase performance by removing a check that is now available in Python 3, but the code you've changed fails in Python 2.

Oh ok, yes, 8250c1785d881f5e711c9a1362034d539ef390cd fails in Python 2, as well as in Python 3.1; so if we would like to maintain python2-compatibility, we should remove 8250c1785d881f5e711c9a1362034d539ef390cd.

So we have conflicting objectives, and I'm asking for developers to decide which objective takes precedence.

Oh I see, I understood it now. Thanks a lot for explaining it again for me :smile: :sweat_smile:

I do not have enough experience to give a decision. If it were me, I would port everything to Python 3 for performance, and I would freeze sugar-toolkit-gtk3, and python3 versions should go along with sugar3-toolkit. The difference in the two version, should ideally be the python3 performance. Maintaining compatibility can be quite complex for newcomer developers too. I am having lack of experience in this part. This is just a suggestion, I am fine with maintaining python2 support

quozl commented 3 years ago

Thanks. I've removed the commits. What information do you have to suggest Python 3 is higher performance for Sugar and activities? In my own tests, Python 3 shows lower performance, which I presume is due to the much larger functionality of Python 3.

chimosky commented 3 years ago

@quozl said

An unrecognised argument for --destdir means my patch isn't applied to the bundle builder being used. There's more than one;

Yes that's how it should be but this happens, your patch was applied and I confirmed it by looking at /usr/lib/python3.7/site-packages/sugar3/activity/bundlebuilder.py and the changes are there.

The exec shows the source you are using is for the Sugar Toolkit for GTK 3 on Python 2, and you should be able to confirm that by reading the first line of setup.py. Where did the source code come from? If it is a git repository, what's the git hash?

A reinstall shows Exec as Sugar Toolkit for GTK3 on Python 2, an install with python3 setup.py install shows Exec as for Sugar Toolkit for GTK3 on Python3 which is expected but I don't understand why a reinstall automatically installs for Python 2. Although it seems to be installing v44 which is shown in the build path that's embedded in the .desktop file after reinstalling where the latest version is v46 which is shown by the debian changelog. Icon = /build/sugar-calculate-activity-44/debian/sugar-calculate-activity//usr/share/sugar/activities/Calculate.activity/activity/calculate.svg

chimosky commented 3 years ago

Downside of 8250c17 and f9b2b6c is withdrawal of Python 2 support by the toolkit. Comments?

Maintaining two separate branches might be good but it's extra work, although we can keep Python 2 images on sunjammer for anyone who needs them. I don't think withdrawing support for Python 2 is great as we still have activities that haven't been ported to Python 3 yet.

quozl commented 3 years ago

@quozl said

An unrecognised argument for --destdir means my patch isn't applied to the bundle builder being used. There's more than one;

Yes that's how it should be but this happens, your patch was applied and I confirmed it by looking at /usr/lib/python3.7/site-packages/sugar3/activity/bundlebuilder.py and the changes are there.

You may apply the patch to the other instances of bundlebuilder.py if you wish. That should entirely fix the unrecognised argument symptom.

The exec shows the source you are using is for the Sugar Toolkit for GTK 3 on Python 2, and you should be able to confirm that by reading the first line of setup.py. Where did the source code come from? If it is a git repository, what's the git hash?

A reinstall shows Exec as Sugar Toolkit for GTK3 on Python 2, an install with python3 setup.py install shows Exec as for Sugar Toolkit for GTK3 on Python3 which is expected but I don't understand why a reinstall automatically installs for Python 2. Although it seems to be installing v44 which is shown in the build path that's embedded in the .desktop file after reinstalling where the latest version is v46 which is shown by the debian changelog. Icon = /build/sugar-calculate-activity-44/debian/sugar-calculate-activity//usr/share/sugar/activities/Calculate.activity/activity/calculate.svg

You're trying to test the bundle builder against an activity source. Where did the source come from? If it is a git repository, what's the git hash? While activity versions can be indicative, you've mentioned two, and you've mentioned derivative source (by Debian project), so I'm not sure what you are using. I also don't know what you are talking about with reinstall; I only mentioned reinstall in my instructions above in order to restore a system to the state it was in before using it for testing. The bundle builder is not involved in Debian package installation, so any behaviours you observed with a reinstall are tangential.

quozl commented 3 years ago

Maintaining two separate branches might be good but it's extra work, although we can keep Python 2 images on sunjammer for anyone who needs them. I don't think withdrawing support for Python 2 is great as we still have activities that haven't been ported to Python 3 yet.

Thanks. Don't worry about the extra work; I already maintain the Fedora 18 branch for all of Sugar and activities under OLPC OS 13.x, and packaging branches for OLPC OS 18 and 20, so if Sugar Labs decides to drop Python 2 support what it really means is that I'll maintain it alone, as I already do for the other branches. Dropping Python 2 support would be a decision by Sugar Labs not to maintain, leaving it to others to do so.

quozl commented 3 years ago

@srevinsaju, did you test this? Did it work for you? Any code review?

chimosky commented 3 years ago

You may apply the patch to the other instances of bundlebuilder.py if you wish. That should entirely fix the unrecognised argument symptom.

I didn't need to at the time as it had already been applied when I built from this branch.

You're trying to test the bundle builder against an activity source. Where did the source come from? If it is a git repository, what's the git hash?

At the time it was from a git repository, with HEAD at sugarlabs/calculate-activity@d36f122.

While activity versions can be indicative, you've mentioned two, and you've mentioned derivative source (by Debian project), so I'm not sure what you are using. I also don't know what you are talking about with reinstall; I only mentioned reinstall in my instructions above in order to restore a system to the state it was in before using it for testing. The bundle builder is not involved in Debian package installation, so any behaviours you observed with a reinstall are tangential.

The source of the activity was a git repository, I shouldn't have mentioned the derivative source but I'd checked and the versions were different which wasn't a surprise as I hadn't updated my test environment at the time; yes I don't expect you to know about the reinstall issue as that's also debian related.

I'll test again and let you know when I do.

quozl commented 3 years ago

Thanks, please do. The data so far conflicts; https://github.com/sugarlabs/calculate-activity/commit/d36f122 has a setup.py of

#!/usr/bin/env python
from sugar3.activity import bundlebuilder
bundlebuilder.start()

yet you got ./setup.py: error: unrecognized arguments: --destdir. Perhaps you were testing on a system where /usr/bin/python is Python 2, and you used ./setup.py instead of python3 setup.py, which implies Python 2. I note that the Debian packaging for the Calculate activity;

We can no longer assume /usr/bin/python to be a specific version of Python, now that downstream distributions have diverged on that. In my cache of about 163 activity repositories, there's a variation in how the Python interpreter is chosen;

#! in setup.py frequency
/usr/bin/python2 1
/usr/bin/python3 4
/usr/bin/python 16
/usr/bin/env python2 0
/usr/bin/env python3 28
/usr/bin/env python 108
chimosky commented 3 years ago

yet you got ./setup.py: error: unrecognized arguments: --destdir. Perhaps you were testing on a system where /usr/bin/python is Python 2, and you used ./setup.py instead of python3 setup.py, which implies Python 2. I note that the Debian packaging for the Calculate activity;

All tests were done with python3 setup.py and I just did it again and I still get same error in same format, I can't debug more at the moment to find out why.

We can no longer assume /usr/bin/python to be a specific version of Python, now that downstream distributions have diverged on that.

Agreed.

quozl commented 3 years ago

The patch adds --destdir support. Therefore the patch must not be applied. Look for evidence of the feature in the bundlebuilder.py file that is executed. You might use strace to prove which file is executed. If you have compiled Python files, make sure to recompile them.

srevinsaju commented 3 years ago

@srevinsaju, did you test this? Did it work for you? Any code review?

Nope, haven't tested this yet. Will surely test it!

quozl commented 3 years ago

Rebased branch after 0.118 release.

srevinsaju commented 3 years ago

Tested, works as expected. Sorry for the delay

chimosky commented 3 years ago

Tested, works as expected.

I couldn't test earlier as I forgot to copy the contents of /usr/lib/python3.7/site-packages/sugar3 to /usr/lib/python3.7/dist-packages.