interpreter: deprecate using an array for project(license:) #9940

dcbaker · 2022-02-03T18:51:28Z

It's ambiguous as to whether the array means AND or OR, and ambiguity in
software licenses is not a good thing. Further, SPDX (which we
generally recommend) is an ISO standard and well understood outside of
Meson (including in SBOM software). Deprecate passing and array and
suggest using an SDPX expression instead.

codecov · 2022-02-03T19:00:11Z

Codecov Report

Merging #9940 (12830bd) into master (29c26d5) will decrease coverage by 1.48%.
The diff coverage is n/a.

❗ Current head 12830bd differs from pull request most recent head af2ae8e. Consider uploading reports for the commit af2ae8e to get more accurate results

@@            Coverage Diff             @@
##           master    #9940      +/-   ##
==========================================
- Coverage   68.71%   67.23%   -1.49%     
==========================================
  Files         406      203     -203     
  Lines       87766    43892   -43874     
  Branches    19518     9762    -9756     
==========================================
- Hits        60311    29512   -30799     
+ Misses      22892    12130   -10762     
+ Partials     4563     2250    -2313

Impacted Files	Coverage Δ
modules/unstable_cuda.py	`0.00% <0.00%> (-72.50%)`	⬇️
templates/cudatemplates.py	`37.50% <0.00%> (-62.50%)`	⬇️
compilers/cuda.py	`19.38% <0.00%> (-45.54%)`	⬇️
dependencies/cuda.py	`20.19% <0.00%> (-42.79%)`	⬇️
compilers/detect.py	`41.11% <0.00%> (-5.04%)`	⬇️
linkers/linkers.py	`56.30% <0.00%> (-1.28%)`	⬇️
environment.py	`78.98% <0.00%> (-1.07%)`	⬇️
dependencies/misc.py	`44.44% <0.00%> (-0.75%)`	⬇️
compilers/compilers.py	`82.18% <0.00%> (-0.47%)`	⬇️
mesonlib/universal.py	`76.45% <0.00%> (-0.32%)`	⬇️
... and 207 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29c26d5...af2ae8e. Read the comment docs.

xclaesse · 2022-02-03T19:13:00Z

LGTM

docs/markdown/snippets/license_as_array.md

eli-schwartz · 2022-02-03T19:19:01Z

mesonbuild/interpreter/interpreter.py

@@ -1135,6 +1135,9 @@ def func_project(self, node: mparser.FunctionNode, args: T.Tuple[str, T.List[str
        if self.build.project_version is None:
            self.build.project_version = self.project_version
        proj_license = kwargs['license']
+        if len(proj_license) > 1:
+            FeatureDeprecated.single_use('license as an array of strings', '0.62.0', self.subproject,


I wonder if we actually need a version-gated feature? We recommend against it purely because it is "a bad idea" and the alternative is to do something that has also worked since this kwarg was added. So there's no reason to only recommend against it once you bump your minimum meson.build high enough.

You're right, the way this is written is bad. I've changed it so that the function takes a str | list[str], and that passing list[str] is deprecated, and the warning is gated on that.

While this does fine tune the check to also trigger on license: ['GPL-2.0-only'] it doesn't really answer my question, which is whether we should use mlog.deprecation and print warnings even for projects that support Meson 0.45, instead of using FeatureDeprecated and only warning people away from this once they bump meson_version: '>=0.62'.

Again, the point of a FeatureDeprecated is typically because we change how things work and add new features which replace old and badly designed features, so as a result a FeatureDeprecated does not warn you if you cannot migrate to the new method due to your requirement to support old versions of Meson.

We also don't warn you at all if you don't specify any minimum version. Whereas a direct mlog.deprecation would be printed rather than ignored.

I've made a change to add a warning in the even that you're using a version < 0.62 in addition to the deprecation warning (either the FeatureDeprecated or the separate warning). The FeatureDeprecated is still useful as passing an array should go away, but you're right, encouraging the use of an SPDX license expression even in older versions is desirable

This seems like a very complicated way to add two distinct warnings for one thing, when passing as an array is still not something that needs to version-gated the warning.

Deprecating a feature with a replacement that has been available for as long as the feature itself, doesn't need a version gate, whether that feature is "multiple license strings or "passing as an array".

We can just tell people using Meson 0.45 "you know, it's kind of bad to not use SPDX, and since you're going to use SPDX, it's kind of silly to pass a single string wrapped in an array, and also this still works on 0.45 anyway (and quite frankly it works on like 0.25 also)".

I'm strongly against removing the FeatureDeprecated, because it provides a clean way for projects receiving bug reports about the deprecation to know exactly what's happening, and when it was deprecated. Having been on the end of CMake policy bug reports/PRs that don't include that information, I think this makes for good user experience. It's immaterial that the replacement has been available since the beginning, 0.62 is when we declared "this is going away".

The other problem we have, is that we don't (and can't easily without external deps) verify that the project license is an SPDX expression, only that it's an array or a single string. That string could easily be LOLZ!1!, and we can't say with certainty "this is not SPDX!". But we can say that ['GPL', 'MIT'] is ambiguous, knock that off.

That would imply we should extend FeatureCheck with a .single_use(..., always_warn=True) option, and just emit the FeatureDeprecated but ensure it is always logged as a full warning.

@dcbaker could you update your PR to add FeatureDeprecated.single_use(..., always_warn=True) please?

See mesonbuild/meson#9940

mesonbuild/interpreter/interpreter.py

This is useful for cases where the replacement has been present for as long (or longer) than the deprecated functionality, but we would still like to signal to users when the given functionality was deprecated.

It's ambiguous as to whether the array means AND or OR, and ambiguity in software licenses is *not* a good thing. Further, SPDX (which we generally recommend) is an ISO standard and well understood outside of Meson (including in SBOM software). Deprecate passing and array and suggest using an SDPX expression instead. If an older version is targeted, then a warning is generated if using an array with a length > 1 to use an SPDX expression instead.

We do check the location as part of the "emit a warning only once" mechanism already, since at least: d39b330

dcbaker · 2022-06-06T17:41:48Z

@xclaesse, @eli-schwartz: I've rebased this, added the requested always_warn flag, added a test, and removed the patch that makes .use return a boolean, as it no longer makes sense. I also noticed a stale FIXME comment that it looks like @eli-schwartz resolved, so I removed that too.

xclaesse · 2022-06-07T12:49:32Z

mesonbuild/interpreterbase/decorators.py

+                'but uses feature deprecated since',
+                f"'{self.feature_version}':",
+                f'{self.feature_name}.',
+            ]


Not sure why we need 2 different messages here, what's wrong with the original one? Also it probably should be fatal when targeting Meson >=0.63 but since that's inside project() I don't think the target meson version is set already, is it?

Sorry for nitpicking, maybe I'm just overthinking this simple PR tbh.

I don't think the normal message makes sense in this case, since you'd get a warning like: Project is targing >= 0.52 but using a feature deprecated since 0.63. That seems confusing to me.

I agree that the dedicated warning message makes sense.

However I also agree with @xclaesse that it seems weird to mark it as non-fatal, because if we always warn about it and can always use the better non-deprecated version of it, then we also do want it to trigger --fatal-meson-warnings.

eli-schwartz · 2022-06-10T03:41:42Z

mesonbuild/interpreterbase/decorators.py

@@ -631,8 +631,6 @@ def use(self, subproject: 'SubProject', location: T.Optional['mparser.BaseNode']
        feature_key = (self.feature_name, location)
        if feature_key in register[self.feature_version]:
            # Don't warn about the same feature multiple times
-            # FIXME: This is needed to prevent duplicate warnings, but also
-            # means we won't warn about a feature used in multiple places.


Good catch, but the actual fix was in 7a033c7.

e.g. the discussion at #9488 (comment)

eli-schwartz · 2022-06-10T03:50:56Z

mesonbuild/interpreterbase/decorators.py

@@ -702,7 +719,7 @@ def log_usage_warning(self, tv: str, location: T.Optional['mparser.BaseNode']) -
        ]
        if self.extra_message:
            args.append(self.extra_message)
-        mlog.warning(*args, location=location)
+        mlog.warning(*args, location=location, fatal=not always)


This should just be dropped. The default is True, but we're setting it to False iff it is an always_warn.

This means that a deprecation for something that is always deprecated and always wrong and always should be replaced... does NOT trigger --fatal-meson-warnings, even though it really should.

dcbaker added this to the 0.62.0 milestone Feb 3, 2022

dcbaker requested a review from jpakkane as a code owner February 3, 2022 18:51

dcbaker force-pushed the submit/deprecate-project-license-array branch from aa5c023 to 7007891 Compare February 3, 2022 19:14

eli-schwartz reviewed Feb 3, 2022

View reviewed changes

docs/markdown/snippets/license_as_array.md Outdated Show resolved Hide resolved

eli-schwartz reviewed Feb 3, 2022

View reviewed changes

dcbaker force-pushed the submit/deprecate-project-license-array branch from 7007891 to ab6b5ef Compare February 3, 2022 19:36

tristan957 mentioned this pull request Feb 3, 2022

Switch license field in project() to str hse-project/hse#199

Merged

dcbaker force-pushed the submit/deprecate-project-license-array branch 2 times, most recently from 80a21c1 to 9dac45f Compare February 4, 2022 18:20

Tachi107 added a commit to Tachi107/cloudflare-ddns that referenced this pull request Feb 25, 2022

build: use SPDX expressions instead of an array

08be98f

See mesonbuild/meson#9940

dcbaker modified the milestones: 0.62.0, 0.63.0 Mar 2, 2022

xclaesse reviewed Apr 30, 2022

View reviewed changes

mesonbuild/interpreter/interpreter.py Outdated Show resolved Hide resolved

dcbaker force-pushed the submit/deprecate-project-license-array branch from 9dac45f to 84630b6 Compare June 6, 2022 17:35

dcbaker added 3 commits June 6, 2022 10:37

decorators: add an "always_warn" flag to Feature(Deprecated|New)

6a17042

This is useful for cases where the replacement has been present for as long (or longer) than the deprecated functionality, but we would still like to signal to users when the given functionality was deprecated.

interpreterbase: delete stale FIXME

af2ae8e

We do check the location as part of the "emit a warning only once" mechanism already, since at least: d39b330

dcbaker force-pushed the submit/deprecate-project-license-array branch from 84630b6 to af2ae8e Compare June 6, 2022 17:41

xclaesse reviewed Jun 7, 2022

View reviewed changes

eli-schwartz reviewed Jun 10, 2022

View reviewed changes

eli-schwartz requested changes Jun 10, 2022

View reviewed changes

dcbaker modified the milestones: 0.63.0, 0.64.0 Jul 5, 2022

Tachi107 mentioned this pull request Jan 16, 2023

Define syntax and format of REUSE.yaml fsfe/reuse-docs#81

Open

nirbheek modified the milestones: 0.64.0, 1.2.0 May 23, 2023

dcbaker removed this from the 1.2.0 milestone Jun 28, 2023

dcbaker added this to the 1.3.0 milestone Jun 28, 2023

xclaesse modified the milestones: 1.3.0, 1.4.0 Oct 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interpreter: deprecate using an array for project(license:) #9940

interpreter: deprecate using an array for project(license:) #9940

dcbaker commented Feb 3, 2022

codecov bot commented Feb 3, 2022 •

edited

Loading

xclaesse commented Feb 3, 2022

eli-schwartz Feb 3, 2022

dcbaker Feb 4, 2022

eli-schwartz Feb 4, 2022 •

edited

Loading

dcbaker Feb 4, 2022

eli-schwartz Feb 4, 2022

dcbaker Feb 4, 2022

eli-schwartz Feb 4, 2022

xclaesse Jun 6, 2022

dcbaker commented Jun 6, 2022

xclaesse Jun 7, 2022

dcbaker Jun 7, 2022

eli-schwartz Jun 10, 2022

eli-schwartz Jun 10, 2022

eli-schwartz Jun 10, 2022

eli-schwartz Jun 10, 2022

interpreter: deprecate using an array for project(license:) #9940

Are you sure you want to change the base?

interpreter: deprecate using an array for project(license:) #9940

Conversation

dcbaker commented Feb 3, 2022

codecov bot commented Feb 3, 2022 • edited Loading

Codecov Report

xclaesse commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-schwartz Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker commented Jun 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 3, 2022 •

edited

Loading

eli-schwartz Feb 4, 2022 •

edited

Loading