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.

Sign up for GitHub

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

Jump to bottom

[Feature Request] Pip Package and Metadata attributes checking to toolkit #369

Open
jatindhankhar opened this issue Jul 18, 2017 · 32 comments
Open

[Feature Request] Pip Package and Metadata attributes checking to toolkit #369

jatindhankhar opened this issue Jul 18, 2017 · 32 comments

Comments

@jatindhankhar
Copy link

Currently toolkit produces a .xo file even when some fields which are standard (but not enforced) like License, summary, categories (inside activity.info and even po files folder are not present. This creates a situation when an activity doesn't even contain metadata like License and a small description. Enforcing it inside the toolkit would allow a common pattern and standardize the activity ecosystem.

Also there is no easy way to install sugar-toolkit, one need to manually copy the necessary files to python distribution folder, a pip package would really help in easing the process of setting up a development environment.

These points were raise during a the weekly GSOC meeting, conversation here
@quozl
Copy link
Contributor

quozl commented Jul 18, 2017

Warnings fine, but enforcing not fine.

AppStream warning already exists, for install target in setup.py. You may use that as an example of how to make warnings. See bundlebuilder.py.

Happy for you to add warnings about missing metadata, but most activity maintainers won't be using the latest toolkit, so the warnings won't be effective. You need a better point of enforcement than the toolkit.

Also remember that anything you do to sugar-toolkit-gtk3 for warnings has to be replicated in sugar-toolkit for the GTK+ 2.0 activities.

Please code your builder to build even with missing metadata, by assuming default values; which for the metadata you list (license, summary, categories) should be straightforward.

po folder will only be present if an activity is internationalised, and not all of them are. Internationalisation of an activity is a much larger task than adding a po folder. No sense in excluding activities for lack of internationalisation, as some have no strings to be internationalised. Please code your builder to build even with missing po folder.

What you have in effect uncovered is the terrible state of activity maintenance, and changes to the toolkit won't do anything for that. Anything that makes activity maintenance harder will reduce maintenance.

Toolkit is installed using make install, not pip. sugar-build and osbuild call make install for chroot development environment. Fedora spec file and Debian packaging files call make install for packaged development environment. make install is a standard GNU target. Making a new install method needs a better justification than wanting an easier way, because then there will be two methods to maintain. You had best spend time understanding the existing methods.

Your issue title mentions Manifest. A file MANIFEST was deprecated a long time ago, instead we use git to list source files for writing the bundle. I think you mean metadata.

@jatindhankhar
Copy link
Author

Correct.

po folder will only be present if an activity is internationalised, and not all of them are. Internationalisation of an activity is a much larger task than adding a po folder. No sense in excluding activities for lack of internationalisation, as some have no strings to be internationalised. Please code your builder to build even with missing po folder.

Please code your builder to build even with missing metadata, by assuming default values; which for the metadata you list (license, summary, categories) should be straightforward.

Maybe you are right regarding that, some web activities don't have them. Point taken
We are enforcing some things at our end. If some activities have missing attributes, bot will show the error to developer and allow him/her to correct. See Following Pull Request jatindhankhar/aslo-v3#5.
We are assuming default values for some fields we can use a special "Unknown " attribute for those.

Toolkit is installed using make install, not pip. sugar-build and osbuild call make install for chroot development environment. Fedora spec file and Debian packaging files call make install for packaged development environment. make install is a standard GNU target. Making a new install method needs a better justification than wanting an easier way, because then there will be two methods to maintain. You had best spend time understanding the existing methods.

Yes. But there is no information and instructions on how to install the toolkits, adding those changes to README.md would be good.

Your issue title mentions Manifest. A file MANIFEST was deprecated a long time ago, instead we use git to list source files for writing the bundle. I think you mean metadata.

Yes I meant metadata, fixing the title

@jatindhankhar jatindhankhar changed the title [Feature Request] Pip Package and Manifest checking to toolkit [Feature Request] Pip Package and Metadata attributes checking to toolkit Jul 18, 2017
@tony37
Copy link

tony37 commented Jul 18, 2017 via email

@walterbender
Copy link
Member

walterbender commented Jul 18, 2017 via email

@jatindhankhar
Copy link
Author

Regarding license, most of the core and popular activities like read, portfolio do have a LICENSE file but then we need to read the license and extract the license (by reading headers),most of them have first header as license name Apache2.0, MIT
but it may get tricky in some cases and increases complexity .
However there are plenty of activities that don't have LICENSE and some don't even build anymore, like this one https://github.com/sugarlabs-test/4572-activity

@quozl
Copy link
Contributor

quozl commented Jul 18, 2017

some don't even build anymore, like this one https://github.com/sugarlabs-test/4572-activity

Looking at your traceback;

20:43:07 worker.1 | File "/activity/setup.py", line 4, in
20:43:07 worker.1 | bundlebuilder.start("JAMultiplos")
20:43:07 worker.1 | TypeError: start() takes no arguments (1 given)

You have somehow used the wrong toolkit. The activity source does import sugar.activity.bundlebuilder, see https://github.com/sugarlabs-test/4572-activity/blob/master/setup.py

The toolkit does accept a bundle name, see https://github.com/sugarlabs/sugar-toolkit/blob/master/src/sugar/activity/bundlebuilder.py#L393

So for that error to appear the wrong toolkit must have been used. Why is that?

@quozl
Copy link
Contributor

quozl commented Jul 18, 2017

For activities with missing license key value in activity.info, they should be reviewed to find what licenses have been used in source files, what license is asserted on the whole by a LICENSE or COPYING file, and then a pull request or commit made against the activity repository to add the license key value to activity.info.

I don't think this is a candidate for automation, but you may be interested in the Debian package licensecheck which can classify source licenses. Meanwhile, the activities ought not to be processed into activities.sugarlabs.org.

The in-production activities.sugarlabs.org only recently acquired a license verification check thanks to @scg's release of source changes to production, so there may be many activities without a license key value in activity.info.

In my cache of activities, the ones without a license key value in activity.info are;

  • Finance,
  • Find Words,
  • Gears,
  • IRC,
  • Letters,
  • Mastermind,
  • Planets, and;
  • PyCut.

Sugar Labs must not publish on activities.sugarlabs.org any bundles without a license or with a license that conflicts with the policy. Same applies for publishing on download.sugarlabs.org or github.com/sugarlabs. So fixing licensing should be a priority, and the best time to do it for an activity is during the next release of an activity.

On the addition of warnings to the dist_xo and dist_source targets of the bundlebuilder, you may be interested in the http://wiki.sugarlabs.org/go/Features page which has a template for thinking through the impact of feature proposals.

@walterbender
Copy link
Member

walterbender commented Jul 19, 2017 via email

@walterbender
Copy link
Member

walterbender commented Jul 19, 2017 via email

@quozl
Copy link
Contributor

quozl commented Jul 19, 2017

Actually, IRC contains purk/COPYING (GPLv2 applied without source copyright header), setup.py (GPLv2+), and ircactivity.py (LGPLv2+). Useful confirming license assessment by Debian

Find Words is at https://github.com/tchx84/find-words-activity and is a combination of MIT, BSD, Apache, and Freesfx.co.uk EULA.

PyCut is at https://github.com/FOSSRIT/PyCut and is Mozilla Public License Version 2.0, with fonts under the Apache license.

@quozl
Copy link
Contributor

quozl commented Jul 19, 2017

@jatindhankhar, please review the following repositories updated README.md files;

Information is somewhat redundant, as it is already in Wiki and packaging downstreams, but as you missed it for sugar-toolkit I've added it.

@jatindhankhar
Copy link
Author

jatindhankhar commented Jul 19, 2017 •
edited
Loading

You have somehow used the wrong toolkit. The activity source does import sugar.activity.bundlebuilder, see https://github.com/sugarlabs-test/4572-activity/blob/master/setup.py
The toolkit does accept a bundle name, see https://github.com/sugarlabs/sugar-toolkit/blob/master/src/sugar/activity/bundlebuilder.py#L393

I used the new gtk3-toolkit https://github.com/sugarlabs/sugar-toolkit-gtk3, I thought majority of activities use gtk3-toolkit , maybe it's one of the old activities. Developer can upload .xo files as well when releasing, so we can skip the building step.
If there are many old activities then we may need to change building module of aslo.

@jatindhankhar, please review the following repositories updated README.md files;
https://github.com/sugarlabs/sugar-datastore sugarlabs/sugar-datastore@de51fff
https://github.com/sugarlabs/sugar-artwork sugarlabs/sugar-artwork@71e8182
https://github.com/sugarlabs/sugar-toolkit sugarlabs/sugar-toolkit@5a590c8
https://github.com/sugarlabs/sugar-toolkit-gtk3 73df533
https://github.com/sugarlabs/sugar sugarlabs/sugar@0c1315c
Information is somewhat redundant, as it is already in Wiki and packaging downstreams, but as you missed it for sugar-toolkit I've added it.

Really helpful. New developers can easily set up development environment.
Thanks a lot 😄
We can also use Github's wiki feature but as you said all the information is already available at Sugar WIki.

I made a script to analyze all the 687 repos in sugar-activities to check for License in activity.info and root directory LICENSE.

Total repos check : 687 
Repos with MetaData License : 647
Repos with LICENSE File : 45
Repos with License entry in file and metadata : 45
Repos with No License : 40

Only 40 repos has no license (some of them are empty repos).
Here is the gist https://gist.github.com/jatindhankhar/ab299b5f318695cfaf6d05fd9ccda058
which contains code as well as the list of activities of each category.

@quozl
Copy link
Contributor

quozl commented Jul 19, 2017

I used the new gtk3-toolkit ... thought majority of activities use gtk3-toolkit

Thanks. I thought majority use gtk2 toolkit. You might extend your script to check setup.py for which import is used; sugar.bundlebuilder or sugar3.bundlebuilder. Also, the file COPYING is commonly used instead of LICENSE.

@jatindhankhar
Copy link
Author

jatindhankhar commented Jul 19, 2017 •
edited
Loading

You might extend your script to check setup.py for which import is used; sugar.bundlebuilder or sugar3.bundlebuilder

Thanks, Will try that 👍

Also, the file COPYING is commonly used instead of LICENSE.

I'll keep that in mind.
I checked all the repos that were flagged as no license, most of them are empty and non empty ones don't have COPYING file or LICENSE file.

@quozl
Copy link
Contributor

quozl commented Jul 19, 2017

Thanks. Don't think you need empty repositories. @i5o, why are they empty? e.g. https://github.com/sugar-activities/4411-activity

@tony37
Copy link

tony37 commented Jul 19, 2017 via email

@jatindhankhar
Copy link
Author

jatindhankhar commented Jul 21, 2017 •
edited
Loading

@quozl I tried to build this activity https://github.com/sugarlabs-test/4726-activity (has no po files) and toolkit cannot build without missing po files.
Terminal cast link, here - https://asciinema.org/a/129957

Dockerfile - https://github.com/jatindhankhar/aslo-v3/blob/extend_build_pipeline/activity-build-docker/Dockerfile

FROM fedora:22

RUN dnf install -y git gettext sugar-toolkit-gtk3 sugar-toolkit

VOLUME /activity
WORKDIR /activity

CMD python /activity/setup.py dist_xo; \
    chmod 777 -R /activity/

@quozl
Copy link
Contributor

quozl commented Jul 21, 2017

You're wrong, check again.

Next time you are reporting a problem, show the step you are having a problem with, not the Docker or other virtualisation wrapping, cloning, using the cd command. Here are the important parts of the problem from your animated log file;

[root@145285e90e3c 4726-activity]# python setup.py dist_xo
WARNING:root:Missing po/ dir, cannot build_locale
[root@145285e90e3c 4726-activity]#

You'll find the bundle in the dist folder, and the exit status is zero. No problem.

And please don't use Fedora 22, it is too old.

@jatindhankhar
Copy link
Author

Sorry, looked again, that was a warning 😞 .

Next time you are reporting a problem, show the step you are having a problem with, not the Docker or other virtualisation wrapping, cloning, using the cd command. Here are the important parts of the problem from your animated log file;

Yes, my intention behind was to show the steps to reproduce and the environment used to build the bundle (same is used by aslo)

And please don't use Fedora 22, it is too old.

Fedora:latest then ?

@quozl
Copy link
Contributor

quozl commented Jul 23, 2017

Sorry, looked again, that was a warning 😞 .

Yes; and by the way that's the type of thing I was proposing earlier in this issue thread; that problems with metadata should cause a warning and not a failure. Are you going to give us a pull request? The exit status of the command is where I would place my trust, not only the stdout or stderr writes.

Fedora:latest then ?

When you use an old Fedora, we're more likely to say "upgrade" rather than do regression or bisection on such an old version of Sugar. Further back you go, the larger the bisection. Fedora 22 uses Sugar 0.104, Fedora 23 uses Sugar 0.106, Fedora 24 uses Sugar 0.108, Fedora 26 uses Sugar 0.110. From Fedora git.

With no clear plans for a Sugar 0.112 yet, my guess is Fedora 26 is adequate for the moment.

I don't know what latest would do. Possibly expose you to new packaging or distribution regressions. 😀

@jatindhankhar
Copy link
Author

Yes; and by the way that's the type of thing I was proposing earlier in this issue thread; that problems with metadata should cause a warning and not a failure. Are you going to give us a pull request? The exit status of the command is where I would place my trust, not only the stdout or stderr writes.

Yes, I made a pull request (jatindhankhar/aslo-v3#7) to aslo-v3 that handles missing po files and missing license(if license is not in activity.info, we extract it from LICENSE or COPYING file, with COPYING having more priority over LICENSE )and other attributes as well.
Mandatory attributes for aslo now are name,bundle_id,icon and activity version.
If we can't find license anywhere then an error explaining the problem is sent to developer via comments.

Only activities that cannot be build by aslo are those which don't have setup.py,
Example:
https://github.com/sugarlabs-test/4530-activity and https://github.com/sugarlabs-test/4788-activity

With no clear plans for a Sugar 0.112 yet, my guess is Fedora 26 is adequate for the moment.

Thanks. Will test and replace 22 with 26. 👍

@quozl
Copy link
Contributor

quozl commented Jul 24, 2017

I meant adding warnings to both toolkits bundlebuilder.py.

The missing setup.py should be patches to those activities.

@jatindhankhar
Copy link
Author

I meant adding warnings to both toolkits bundlebuilder.py.

Oh 😅
Taking a look at them. Sorry for the confusion.

@jatindhankhar
Copy link
Author

jatindhankhar commented Jul 24, 2017 •
edited by quozl
Loading

@quozl, I was looking at the bundlebuilder.py https://github.com/sugarlabs/sugar-toolkit-gtk3/blob/master/src/sugar3/activity/bundlebuilder.py#L364

        required_fields = ['metadata_license', 'license', 'name', 'icon',
                           'description']
        for name in required_fields:
            if not info.has_option('Activity', name):
                print('[WARNING] Activity needs more metadata for AppStream '
                      'file')
                print('  Without an AppStream file, the activity will NOT '
                      'show in software stores!')
                print('  Please `pydoc sugar3.activity.bundlebuilder` for'
                      'more info')
                return

So I just need to add fields like bundle_id, summary and it seems like toolkit already warns the developer

@quozl
Copy link
Contributor

quozl commented Jul 24, 2017

You originally said license, summary and categories, and not bundle_id.

As you're suggesting summary and categories as mandatory metadata, whereas previously they were optional, please also;

Hope you can see from the above why we're all resistant to changes to the metadata format. 😁

(You may sense an inconsistency; declaring the metadata mandatory in documentation yet implementing a check that does not fail the dist_xo step is so that activities will continue to build; there are a considerable number of activities not in aslo, but I don't have the numbers. Breaking these activities in next toolkit release would be unfriendly of us.)

So I just need to add fields like bundle_id, summary and it seems like toolkit already warns the developer

My guess is you haven't tried this yet, or you would have found out why it would not work;

  • the code you have identified is in the install target, which is used by downstream packaging and not generating the .xo file, so it is the wrong place to put the check,

  • the warning message text will be wrong; it is not for AppStream metadata that you need categories and summary, but for aslo-v3.

  • adding to this warning would violate the method naming; _generate_appdata is for AppStream metadata, not aslo-v3 metadata,

  • even if the install target was used, the warning may not occur; the call to _generate_appdata is skipped if the keyword parameter install_desktop_file is set to True.

Also;

  • you don't need a check for bundle_id, because without it the dist_xo target fails, see traceback

  • you will need similar checks for the GTK+ 2 toolkit as well.

You might extend your script to check setup.py for which import is used; sugar.bundlebuilder or sugar3.bundlebuilder

Thanks, Will try that 👍

Was there any result? Knowing the number of GTK+ 2 activities will help prioritise any GTK+ 2 toolkit changes.

@jatindhankhar
Copy link
Author

you will need similar checks for the GTK+ 2 toolkit as well.

You might extend your script to check setup.py for which import is used; sugar.bundlebuilder or sugar3.bundlebuilder
Thanks, Will try that 👍
Was there any result? Knowing the number of GTK+ 2 activities will help prioritise any GTK+ 2 toolkit changes.

Yes, GTK+ 2 activities are building fine. Although I do need to test the core activities as well. I just installed old toolkit and setup.py automatically loads the correct toolkit.
I don't have any numbers regarding split between old and new setup and no separate checks are needed for GTK+2, both follow the same pipeline.
Only thing that was dropped was the --no-fail parameter which is not supported by toolkit so it was dropped.

As you're suggesting summary and categories as mandatory metadata, whereas

previously they were optional, please also;

update the sugar3.bundle module which declares the bundle format, see it as rendered documentation
update the oft cited sample Hello World activity, which does not have summary or categories,
update the Wiki declaration of the activity.info file format,
update the Write your own Sugar desktop activity page in the developer documentation, see desktop-activity.md, which does not have summary or categories, and;
post to sugar-devel@ advising the less active developers of the change and why it is happening; because you can't expect them to have noticed your posts about aslo-v3 or this issue #369.

As for the parameters. Majority of the activities that I encountered use summary and very few use description.
If we don't want to break the old activities, we can use move them to optional metadata keys ?
I haven't test them, so my points are more of wild guesses.
Yes, that looks like at lot of work. I will try them after testing core activities with Fedora 26. Thanks for the pointers.

@jatindhankhar jatindhankhar mentioned this issue Jul 25, 2017
Merged
@quozl
Copy link
Contributor

quozl commented Jul 25, 2017

What do you mean by core activities? Perhaps you mean the Fructose set. Or maybe the set packaged by Fedora. Or maybe the set packaged by Debian. Or maybe the set chosen by OLPC.

I still think you should extend your survey script to check setup.py for which import is used, and give us the results. It would be very helpful to know how much effort to spend on the GTK+ 2 vs GTK+ 3 toolkits. You're in a good position to do this right now.

--no-fail ignores errors from msgfmt when building locale, yet doesn't ignore missing po/. Depending on the number of GTK+ 2 activities you might port --no-fail back to sugar-toolkit. You might also change it to ignore missing po/ in both toolkits.

What has the description metadata key got to do with this? It isn't in the Sugar activity.info file format, and is an optional key used by the two activities that support AppStream metadata; Browse and Speak. Only downstreams such as Fedora and Debian use the install argument to setup.py, and deal with the AppStream XML file, as can be seen in the Fedora sugar-browse.spec file. description was introduced in Sugar 0.110 and ought to be considered very new; it has not been widely adopted. I don't think you should be relying on it at all. How many activities have the description key?

You've twice mentioned new metadata keys that you want; could you please spend a moment giving a definitive list?

@quozl
Copy link
Contributor

quozl commented Jul 25, 2017

What do you mean by core activities?

Ah, in jatindhankhar/aslo-v3#8 you referred to the sugar-build set of activities as core. No end-user uses sugar-build, so please don't use that set definition.

@jatindhankhar
Copy link
Author

jatindhankhar commented Jul 26, 2017 •
edited by quozl
Loading

Ah, in jatindhankhar/aslo-v3#8 you referred to the sugar-build set of activities as core. No end-user uses sugar-build, so please don't use that set definition.

Yes but some of the activities like Chat, Log, Image Viewer, Write, Jukebox, Pippy are also part of the Fructose set and they all work fine.

I still think you should extend your survey script to check setup.py for which import is used, and give us the results. It would be very helpful to know how much effort to spend on the GTK+ 2 vs GTK+ 3 toolkits. You're in a good position to do this right now.

On it.

What has the description metadata key got to do with this? It isn't in the Sugar activity.info file format, and is an optional key used by the two activities that support AppStream metadata; Browse and Speak. Only downstreams such as Fedora and Debian use the install argument to setup.py, and deal with the AppStream XML file, as can be seen in the Fedora sugar-browse.spec file. description was introduced in Sugar 0.110 and ought to be considered very new; it has not been widely adopted. I don't think you should be relying on it at all. How many activities have the description key?

This also needs to be surveyed, adding it to script. Aslo already uses summary field.

--no-fail ignores errors from msgfmt when building locale, yet doesn't ignore missing po/. Depending on the number of GTK+ 2 activities you might port --no-fail back to sugar-toolkit. You might also change it to ignore missing po/ in both toolkits.

Thanks, I will try that (after adding some features to aslo).

@jatindhankhar
Copy link
Author

So, I modified the script and here the are the results.

Total repos check : 687
Repos with MetaData License : 646
Repos with LICENSE File : 45
Repos with License entry in file and metadata : 45
Repos with Old Toolkit : 382
Repos with New Toolkit : 98
Repos with summary : 647
Repos with description : 0

Most of the activities use old toolkit and no activity uses description and most (not all) use summary.

Here is the new gist https://gist.github.com/jatindhankhar/2fe548212257ad5911ac418547ede434

@quozl
Copy link
Contributor

quozl commented Jul 26, 2017

Thanks. About 75% GTK+ 2. Smaller than I had feared, but still very large. So I'm happy to take any feature backports from the GTK+ 3 toolkit to the GTK+ 2 toolkit. 😁

Fascinating that there are 207 repositories with no setup.py file. On the other hand it was always possible to make a bundle without the setup.py file, just by using zip. Made it easier for many developers, because they didn't need to install the Sugar toolkit on their development system. Some of those bundles may contain *.py files with from sugar3. vs from sugar.

@jatindhankhar
Copy link
Author

I modified the script. There are 201 such activities
https://gist.github.com/jatindhankhar/2fe548212257ad5911ac418547ede434#file-has_no_setup-txt

On the other hand it was always possible to make a bundle without the setup.py file, just by using zip. Made it easier for many developers, because they didn't need to install the Sugar toolkit on their development system

Aslo-v3 supports bundled .xo, so developer can just upload buit .xo and aslo-v3 will process it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@tony37 @jatindhankhar @quozl @walterbender