Add setup.py --destdir support #454

quozl · 2020-12-02T00:33:33Z

simplify messages,
add desktop and appdata files to messages,
add DESTDIR support.

chimosky · 2020-12-03T00:20:35Z

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 · 2020-12-03T04:21:01Z

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

prepare a system with Ubuntu 20.04 and Sugar 0.117, and apply the patch,
manually remove the files that had been installed by the sugar-calculate-activity package, but without removing the package,

rm -rvf \
/usr/share/applications/org.laptop.Calculate.activity.desktop \
/usr/share/doc/sugar-calculate-activity \
/usr/share/locale/*/LC_MESSAGES/org.laptop.Calculate.mo \
/usr/share/sugar/activities/Calculate.activity

verify the activity is absent from the Home View,
clone the Calculate activity and patch metadata so that the appdata code path is tested;

--- a/activity/activity.info
+++ b/activity/activity.info
@@ -10,3 +10,5 @@ summary = This ...
 repository = https://github.com/sugarlabs/sugarlabs-calculate
 url = https://help.sugarlabs.org/calculate.html
 tags = Maths
+metadata_license = foo
+description = bar

test install step without options, with --prefix, with --destdir, and with both, each time verifying the output.

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`

removing all files added by the test, and then reinstalling the sugar-calculate-activity package,

rm -rvf /usr/share/applications/org.laptop.Calculate.activity.desktop \
/usr/share/doc/sugar-calculate-activity \
/usr/share/locale/*/LC_MESSAGES/org.laptop.Calculate.mo \
/usr/share/sugar/activities/Calculate.activity \
/usr/share/metainfo/org.laptop.Calculate.appdata.xml
rm -rvf /usr/local/share/applications/org.laptop.Calculate.activity.desktop \
/usr/local/share/doc/sugar-calculate-activity \
/usr/local/share/locale/*/LC_MESSAGES/org.laptop.Calculate.mo \
/usr/local/share/sugar/activities/Calculate.activity \
/usr/local/share/metainfo/org.laptop.Calculate.appdata.xml
rm -rvf /tmp/target
apt install --reinstall sugar-calculate-activity

verify the activity is present in the Home View,
reverse the patch.

chimosky · 2020-12-03T12:41:49Z

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 · 2020-12-03T21:50:22Z

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

the Sugar Toolkit for GTK 2,
the Sugar Toolkit for GTK 3 on Python 2,
the Sugar Toolkit for GTK 3 on Python 3.

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 · 2020-12-05T02:28:41Z

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

walterbender · 2020-12-05T02:57:16Z

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

quozl · 2020-12-05T03:05:44Z

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 · 2020-12-05T10:18:39Z

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

quozl · 2020-12-06T20:57:26Z

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 · 2020-12-06T21:31:03Z

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 · 2020-12-07T04:12:24Z

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.

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

srevinsaju · 2020-12-07T04:49:47Z

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, 8250c17 fails in Python 2, as well as in Python 3.1; so if we would like to maintain python2-compatibility, we should remove 8250c17.

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 😄 😅

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 · 2020-12-07T06:10:17Z

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 · 2020-12-07T10:52:08Z

@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 · 2020-12-07T10:54:56Z

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 · 2020-12-07T21:16:52Z

@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 · 2020-12-07T21:35:09Z

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 · 2020-12-09T06:03:21Z

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

chimosky · 2020-12-11T13:35:32Z

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 · 2020-12-11T22:30:21Z

Thanks, please do. The data so far conflicts; sugarlabs/calculate-activity@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;

https://salsa.debian.org/pkg-sugar-team/sugar-calculate-activity/-/blob/master/debian/rules#L3 uses cdbs, and;
https://salsa.debian.org/build-common-team/cdbs/-/blob/master/1/class/python-sugar.mk.in#L119 explicitly calls setup.py from within python3 not python; it ignores what setup.py specifies.

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 · 2020-12-17T11:33:45Z

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 · 2020-12-17T20:06:24Z

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 · 2020-12-17T20:07:49Z

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

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

- remove the trailing period, - remove the source path,

Add support for DESTDIR using a --destdir option, https://www.gnu.org/prep/standards/html_node/DESTDIR.html Fixes sugarlabs#453 Fixes https://bugs.debian.org/975373 Fixes https://bugs.debian.org/975374

quozl · 2020-12-18T05:44:59Z

Rebased branch after 0.118 release.

srevinsaju · 2020-12-28T21:32:20Z

Tested, works as expected. Sorry for the delay

chimosky · 2021-01-08T13:29:50Z

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.

quozl mentioned this pull request Dec 2, 2020

bundle setup.py install with --destdir #453

Closed

srevinsaju mentioned this pull request Dec 4, 2020

perf: use os.makedirs param, exist_ok instead of manually checking if the resultant directory exists quozl/sugar-toolkit-gtk3#1

Closed

quozl force-pushed the is453 branch from 8250c17 to 49a154b Compare December 7, 2020 06:09

quozl added 4 commits December 18, 2020 16:44

bundlebuilder: install: simplify messages

29db666

- remove the trailing period, - remove the source path,

bundlebuilder: install: add desktop path to messages

0c83230

bundlebuilder: install: add appdata path to messages

8988ab4

bundlebuilder: install: add destdir support

4d1b8e8

Add support for DESTDIR using a --destdir option, https://www.gnu.org/prep/standards/html_node/DESTDIR.html Fixes sugarlabs#453 Fixes https://bugs.debian.org/975373 Fixes https://bugs.debian.org/975374

quozl force-pushed the is453 branch from 49a154b to 4d1b8e8 Compare December 18, 2020 05:44

quozl marked this pull request as ready for review December 28, 2020 22:37

srevinsaju approved these changes Jan 3, 2021

View reviewed changes

chimosky merged commit db642e1 into sugarlabs:master Jan 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setup.py --destdir support #454

Add setup.py --destdir support #454

quozl commented Dec 2, 2020

chimosky commented Dec 3, 2020

quozl commented Dec 3, 2020 •

edited

Loading

chimosky commented Dec 3, 2020 •

edited

Loading

quozl commented Dec 3, 2020

quozl commented Dec 5, 2020

walterbender commented Dec 5, 2020

quozl commented Dec 5, 2020

srevinsaju commented Dec 5, 2020

quozl commented Dec 6, 2020

srevinsaju commented Dec 6, 2020

quozl commented Dec 7, 2020

srevinsaju commented Dec 7, 2020 •

edited

Loading

quozl commented Dec 7, 2020

chimosky commented Dec 7, 2020

chimosky commented Dec 7, 2020

quozl commented Dec 7, 2020 •

edited

Loading

quozl commented Dec 7, 2020

quozl commented Dec 9, 2020

chimosky commented Dec 11, 2020

quozl commented Dec 11, 2020

chimosky commented Dec 17, 2020

quozl commented Dec 17, 2020

srevinsaju commented Dec 17, 2020

quozl commented Dec 18, 2020

srevinsaju commented Dec 28, 2020 •

edited

Loading

chimosky commented Jan 8, 2021

Add setup.py --destdir support #454

Add setup.py --destdir support #454

Conversation

quozl commented Dec 2, 2020

chimosky commented Dec 3, 2020

quozl commented Dec 3, 2020 • edited Loading

chimosky commented Dec 3, 2020 • edited Loading

quozl commented Dec 3, 2020

quozl commented Dec 5, 2020

walterbender commented Dec 5, 2020

quozl commented Dec 5, 2020

srevinsaju commented Dec 5, 2020

quozl commented Dec 6, 2020

srevinsaju commented Dec 6, 2020

quozl commented Dec 7, 2020

srevinsaju commented Dec 7, 2020 • edited Loading

quozl commented Dec 7, 2020

chimosky commented Dec 7, 2020

chimosky commented Dec 7, 2020

quozl commented Dec 7, 2020 • edited Loading

quozl commented Dec 7, 2020

quozl commented Dec 9, 2020

chimosky commented Dec 11, 2020

quozl commented Dec 11, 2020

chimosky commented Dec 17, 2020

quozl commented Dec 17, 2020

srevinsaju commented Dec 17, 2020

quozl commented Dec 18, 2020

srevinsaju commented Dec 28, 2020 • edited Loading

chimosky commented Jan 8, 2021

quozl commented Dec 3, 2020 •

edited

Loading

chimosky commented Dec 3, 2020 •

edited

Loading

srevinsaju commented Dec 7, 2020 •

edited

Loading

quozl commented Dec 7, 2020 •

edited

Loading

srevinsaju commented Dec 28, 2020 •

edited

Loading