| Message ID | 20240210111451.2112513-2-pkj@axis.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/2] license.py: Add available_licenses() | expand |
On Sat, 2024-02-10 at 12:14 +0100, Peter Kjellerstedt wrote: > Contrary to INCOMPATIBLE_LICENSE, which is used to define a few unwanted > licenses, this class takes the opposite stance and expects > COMPATIBLE_LICENSES, which contains a list of all allowed licenses. Any > license that is not in COMPATIBLE_LICENSES, will be added to > INCOMPATIBLE_LICENSE. > > The typical use case for this class is if you, e.g., have a company > policy that declares what licenses are allowed to be used. By using this > class and defining COMPATIBLE_LICENSES to contain the list of those > licenses, any new code that is added with a license that has not (yet) > been approved will fail until the license is approved and added to > COMPATIBLE_LICENSES. > > The class is intended to be inherited by image recipes, e.g., by adding > it to IMAGE_CLASSES. > > Since it is still INCOMPATIBLE_LICENSE that is used in the end, you can > override individual packages using INCOMPATIBLE_LICENSE_EXCEPTIONS as > usual. > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com> > --- > > Someone asked for a way to only have to define the compatible licenses a > way back (actually, quite a way back) and this is the class that we have > been using to solve this. > > Compared to an old solution that used AVAILABLE_LICENSES, which was > removed in commit bf08d9ccb9cbc749a571af3d33140bcae0e252a6, this > solution should have none of the drawbacks. Since this class is only > expected to be inherited from image recipes, there is only a neglible > performance impact, and LICENSE_PATH, which is used by > oe.license.available_licenses(), should be well defined at that time. > > I will send additional patches for the documentation if (as I hope) the > class is accepted. > > .../compatible-licenses.bbclass | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) > create mode 100644 meta/classes-recipe/compatible-licenses.bbclass > > diff --git a/meta/classes-recipe/compatible-licenses.bbclass b/meta/classes-recipe/compatible-licenses.bbclass > new file mode 100644 > index 0000000000..46f72f831f > --- /dev/null > +++ b/meta/classes-recipe/compatible-licenses.bbclass > @@ -0,0 +1,29 @@ > +# > +# Copyright OpenEmbedded Contributors > +# > +# SPDX-License-Identifier: MIT > +# > + > +# Contrary to INCOMPATIBLE_LICENSE, which is used to define a few unwanted > +# licenses, this class takes the opposite stance and expects > +# COMPATIBLE_LICENSES, which contains a list of all allowed licenses. Any > +# license that is not in COMPATIBLE_LICENSES, will be added to > +# INCOMPATIBLE_LICENSE. > +# > +# The typical use case for this class is if you, e.g., have a company policy > +# that declares what licenses are allowed to be used. By using this class and > +# defining COMPATIBLE_LICENSES to contain the list of those licenses, any new > +# code that is added with a license that has not (yet) been approved will fail > +# until the license is approved and added to COMPATIBLE_LICENSES. > +# > +# The class is intended to be inherited by image recipes, e.g., by adding it to > +# IMAGE_CLASSES. > +# > +# Since it is still INCOMPATIBLE_LICENSE that is used in the end, you can > +# override individual packages using INCOMPATIBLE_LICENSE_EXCEPTIONS as usual. > + > +COMPATIBLE_LICENSES ??= "" > +COMPATIBLE_LICENSES[doc] = "Specifies a space-separated list of all licenses that are allowed in the build." > + > +# Mark all licenses that are not in COMPATIBLE_LICENSES as incompatible. > +INCOMPATIBLE_LICENSE += "${@' '.join(sorted(oe.license.available_licenses(d) - set(d.getVar('COMPATIBLE_LICENSES').split())))}" I strongly disagree with this. Obviously when I'm struggling with the load of patches and the backlog and we're about a week before feature freeze, now is a really great time to bring this back up again, thanks for that. I now need to find the time to respond. Whilst I think the use case for saying "I don't like license XXX" is clear and we called than "incompatible" for want of a better description, I don't think the concept of "compatible" is as clear. The issue is that it isn't clear what it is meant to be compatible with. As such I do concerns about the naming but I do understand how we end up with it and I haven't thought of what could work better. The implications of the way this is implemented still worries me as you're having any recipe using it make multiple directory listings during parsing through an inline python call. Users won't realise that is going to be fairly slow but they are going to complain parsing is slow. Somehow we need to try and speed up parsing, not slow it down further with this kind of operation. This change makes the hit per recipe, again, I understand why but I don't have to like it. There is a much simpler implementation possible where the code simply checks if the license is in the list and then fails if it isn't. Trying to make this work off the back of INCOMPATIBLE license just feels wrong to me. The code was previously removed as a way to try and simplify this, give us some chance of being able to improve parsing. I don't think this patch series does address the concerns back then. So no, presenting basically the same code again doesn't really work for me, sorry. Cheers, Richard
> -----Original Message----- > From: Richard Purdie <richard.purdie@linuxfoundation.org> > Sent: den 10 februari 2024 12:59 > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-core@lists.openembedded.org > Subject: Re: [OE-core] [RFC][PATCH 2/2] compatible-licenses.bbclass: Add class > > On Sat, 2024-02-10 at 12:14 +0100, Peter Kjellerstedt wrote: > > Contrary to INCOMPATIBLE_LICENSE, which is used to define a few unwanted > > licenses, this class takes the opposite stance and expects > > COMPATIBLE_LICENSES, which contains a list of all allowed licenses. Any > > license that is not in COMPATIBLE_LICENSES, will be added to > > INCOMPATIBLE_LICENSE. > > > > The typical use case for this class is if you, e.g., have a company > > policy that declares what licenses are allowed to be used. By using this > > class and defining COMPATIBLE_LICENSES to contain the list of those > > licenses, any new code that is added with a license that has not (yet) > > been approved will fail until the license is approved and added to > > COMPATIBLE_LICENSES. > > > > The class is intended to be inherited by image recipes, e.g., by adding > > it to IMAGE_CLASSES. > > > > Since it is still INCOMPATIBLE_LICENSE that is used in the end, you can > > override individual packages using INCOMPATIBLE_LICENSE_EXCEPTIONS as > > usual. > > > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com> > > --- > > > > Someone asked for a way to only have to define the compatible licenses a > > way back (actually, quite a way back) and this is the class that we have > > been using to solve this. > > > > Compared to an old solution that used AVAILABLE_LICENSES, which was > > removed in commit bf08d9ccb9cbc749a571af3d33140bcae0e252a6, this > > solution should have none of the drawbacks. Since this class is only > > expected to be inherited from image recipes, there is only a neglible > > performance impact, and LICENSE_PATH, which is used by > > oe.license.available_licenses(), should be well defined at that time. > > > > I will send additional patches for the documentation if (as I hope) the > > class is accepted. > > > > .../compatible-licenses.bbclass | 29 +++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > create mode 100644 meta/classes-recipe/compatible-licenses.bbclass > > > > diff --git a/meta/classes-recipe/compatible-licenses.bbclass b/meta/classes-recipe/compatible-licenses.bbclass > > new file mode 100644 > > index 0000000000..46f72f831f > > --- /dev/null > > +++ b/meta/classes-recipe/compatible-licenses.bbclass > > @@ -0,0 +1,29 @@ > > +# > > +# Copyright OpenEmbedded Contributors > > +# > > +# SPDX-License-Identifier: MIT > > +# > > + > > +# Contrary to INCOMPATIBLE_LICENSE, which is used to define a few unwanted > > +# licenses, this class takes the opposite stance and expects > > +# COMPATIBLE_LICENSES, which contains a list of all allowed licenses. Any > > +# license that is not in COMPATIBLE_LICENSES, will be added to > > +# INCOMPATIBLE_LICENSE. > > +# > > +# The typical use case for this class is if you, e.g., have a company policy > > +# that declares what licenses are allowed to be used. By using this class and > > +# defining COMPATIBLE_LICENSES to contain the list of those licenses, any new > > +# code that is added with a license that has not (yet) been approved will fail > > +# until the license is approved and added to COMPATIBLE_LICENSES. > > +# > > +# The class is intended to be inherited by image recipes, e.g., by adding it to > > +# IMAGE_CLASSES. > > +# > > +# Since it is still INCOMPATIBLE_LICENSE that is used in the end, you can > > +# override individual packages using INCOMPATIBLE_LICENSE_EXCEPTIONS as usual. > > + > > +COMPATIBLE_LICENSES ??= "" > > +COMPATIBLE_LICENSES[doc] = "Specifies a space-separated list of all licenses that are allowed in the build." > > + > > +# Mark all licenses that are not in COMPATIBLE_LICENSES as incompatible. > > +INCOMPATIBLE_LICENSE += "${@' '.join(sorted(oe.license.available_licenses(d) - set(d.getVar('COMPATIBLE_LICENSES').split())))}" > > I strongly disagree with this. > > Obviously when I'm struggling with the load of patches and the backlog > and we're about a week before feature freeze, now is a really great > time to bring this back up again, thanks for that. I now need to find > the time to respond. I was going through different local changes and found this, and remembered I had promised someone that I would submit it upstream (but I have since forgotten who asked for it). Since I thought it may be useful to others who have the same situation we do with a small set of compatible licenses rather than a small set of incompatible licenses, I thought it would be worth to contribute it. And since the class is very small and simple, I thought it might even stand a chance to be accepted for Scarthgap (but I was obviously wrong). Now, I could try to defend the submission and its implementation, but since I think that is rather moot given the circumstances, I will instead focus on the opening for a better solution you give below (which obviously would be for post-Scarthgap). > > Whilst I think the use case for saying "I don't like license XXX" is > clear and we called than "incompatible" for want of a better > description, I don't think the concept of "compatible" is as clear. The > issue is that it isn't clear what it is meant to be compatible with. As > such I do concerns about the naming but I do understand how we end up > with it and I haven't thought of what could work better. > > The implications of the way this is implemented still worries me as > you're having any recipe using it make multiple directory listings > during parsing through an inline python call. Users won't realise that > is going to be fairly slow but they are going to complain parsing is > slow. Somehow we need to try and speed up parsing, not slow it down > further with this kind of operation. This change makes the hit per > recipe, again, I understand why but I don't have to like it. > > There is a much simpler implementation possible where the code simply > checks if the license is in the list and then fails if it isn't. Trying > to make this work off the back of INCOMPATIBLE license just feels wrong > to me. I would be happy to implement such a solution, as I totally agree that it would be much more efficient and completely alleviates the need for the available_licenses() function and its drawbacks. I will not have time to look at this until after Scarthgap has been released, however, so we can wait with any further discussions on how it can be done till then. But I will leave a couple of questions that will need to be answered eventually: * When should COMPATIBLE_LICENSES take effect? The simplest solution (to me) is that it does noting until at least one license is added to it (since otherwise every license would be considered incompatible and nothing could be built, which would be rather pointless). * Are there better names than COMPATIBLE_LICENSES and INCOMPATIBLE_LICENSE? To me it seems pretty obvious what they mean, but given your concerns above, I am not so sure anymore. * Should COMPATIBLE_LICENSES and INCOMPATIBLE_LICENSE be mutually exclusive, or is it ok to use both at the same time? I can see a need to be able to use both. E.g., if COMPATIBLE_LICENSES is used to define what licenses are ok to use in an image, one may still want to be able to mark *GPL-3* as incompatible globally so that all such recipes are skipped and recipes with older versions can take over (the way meta-gpl2 worked). * What to do if a license is marked as both compatible and incompatible? Here it seems best to take the safer stance and let incompatible win, as the consequences of using code with an incompatible license are much more dire than not being able to use it. There will probably be more, once I start looking into actually implementing it. > > The code was previously removed as a way to try and simplify this, give > us some chance of being able to improve parsing. I don't think this > patch series does address the concerns back then. > > So no, presenting basically the same code again doesn't really work for > me, sorry. > > Cheers, > > Richard //Peter
diff --git a/meta/classes-recipe/compatible-licenses.bbclass b/meta/classes-recipe/compatible-licenses.bbclass new file mode 100644 index 0000000000..46f72f831f --- /dev/null +++ b/meta/classes-recipe/compatible-licenses.bbclass @@ -0,0 +1,29 @@ +# +# Copyright OpenEmbedded Contributors +# +# SPDX-License-Identifier: MIT +# + +# Contrary to INCOMPATIBLE_LICENSE, which is used to define a few unwanted +# licenses, this class takes the opposite stance and expects +# COMPATIBLE_LICENSES, which contains a list of all allowed licenses. Any +# license that is not in COMPATIBLE_LICENSES, will be added to +# INCOMPATIBLE_LICENSE. +# +# The typical use case for this class is if you, e.g., have a company policy +# that declares what licenses are allowed to be used. By using this class and +# defining COMPATIBLE_LICENSES to contain the list of those licenses, any new +# code that is added with a license that has not (yet) been approved will fail +# until the license is approved and added to COMPATIBLE_LICENSES. +# +# The class is intended to be inherited by image recipes, e.g., by adding it to +# IMAGE_CLASSES. +# +# Since it is still INCOMPATIBLE_LICENSE that is used in the end, you can +# override individual packages using INCOMPATIBLE_LICENSE_EXCEPTIONS as usual. + +COMPATIBLE_LICENSES ??= "" +COMPATIBLE_LICENSES[doc] = "Specifies a space-separated list of all licenses that are allowed in the build." + +# Mark all licenses that are not in COMPATIBLE_LICENSES as incompatible. +INCOMPATIBLE_LICENSE += "${@' '.join(sorted(oe.license.available_licenses(d) - set(d.getVar('COMPATIBLE_LICENSES').split())))}"
Contrary to INCOMPATIBLE_LICENSE, which is used to define a few unwanted licenses, this class takes the opposite stance and expects COMPATIBLE_LICENSES, which contains a list of all allowed licenses. Any license that is not in COMPATIBLE_LICENSES, will be added to INCOMPATIBLE_LICENSE. The typical use case for this class is if you, e.g., have a company policy that declares what licenses are allowed to be used. By using this class and defining COMPATIBLE_LICENSES to contain the list of those licenses, any new code that is added with a license that has not (yet) been approved will fail until the license is approved and added to COMPATIBLE_LICENSES. The class is intended to be inherited by image recipes, e.g., by adding it to IMAGE_CLASSES. Since it is still INCOMPATIBLE_LICENSE that is used in the end, you can override individual packages using INCOMPATIBLE_LICENSE_EXCEPTIONS as usual. Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com> --- Someone asked for a way to only have to define the compatible licenses a way back (actually, quite a way back) and this is the class that we have been using to solve this. Compared to an old solution that used AVAILABLE_LICENSES, which was removed in commit bf08d9ccb9cbc749a571af3d33140bcae0e252a6, this solution should have none of the drawbacks. Since this class is only expected to be inherited from image recipes, there is only a neglible performance impact, and LICENSE_PATH, which is used by oe.license.available_licenses(), should be well defined at that time. I will send additional patches for the documentation if (as I hope) the class is accepted. .../compatible-licenses.bbclass | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 meta/classes-recipe/compatible-licenses.bbclass