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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a17042
decorators: add an "always_warn" flag to Feature(Deprecated|New)
dcbaker Jun 6, 2022
46fca7c
interpreter: deprecate using an array for project(license:)
dcbaker Feb 3, 2022
af2ae8e
interpreterbase: delete stale FIXME
dcbaker Jun 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 41 additions & 14 deletions mesonbuild/interpreterbase/decorators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,8 +600,21 @@ def get_target_version(subproject: str) -> str:
def check_version(target_version: str, feature_version: str) -> bool:
pass

def use(self, subproject: 'SubProject', location: T.Optional['mparser.BaseNode'] = None) -> None:
def use(self, subproject: 'SubProject', location: T.Optional['mparser.BaseNode'] = None,
always_warn: bool = False) -> None:
"""Generate a warning if necessary.

:param subproject: The subproject this warning is for
:param location: The location node that generated the warning, defaults to None
:param always_warn: If set to True a warning is always generated.
This is useful for cases where the deprecated behavior has always
had a replacement, but we still wish to signal the version in which
the behavior was deprecated. Defaults to False
"""
tv = self.get_target_version(subproject)
if always_warn:
self.log_usage_warning(tv, location, always=always_warn)
return
# No target version
if tv == '':
return
Expand Down Expand Up @@ -645,7 +658,8 @@ def report(cls, subproject: str) -> None:
if '\n' in warning_str:
mlog.warning(warning_str)

def log_usage_warning(self, tv: str, location: T.Optional['mparser.BaseNode']) -> None:
def log_usage_warning(self, tv: str, location: T.Optional['mparser.BaseNode'],
always: bool = False) -> None:
raise InterpreterException('log_usage_warning not implemented')

@staticmethod
Expand All @@ -668,9 +682,10 @@ def wrapped(*wrapped_args: T.Any, **wrapped_kwargs: T.Any) -> T.Any:

@classmethod
def single_use(cls, feature_name: str, version: str, subproject: 'SubProject',
extra_message: str = '', location: T.Optional['mparser.BaseNode'] = None) -> None:
extra_message: str = '', location: T.Optional['mparser.BaseNode'] = None,
always_warn: bool = False) -> None:
"""Oneline version that instantiates and calls use()."""
cls(feature_name, version, extra_message).use(subproject, location)
cls(feature_name, version, extra_message).use(subproject, location, always_warn)


class FeatureNew(FeatureCheckBase):
Expand All @@ -693,7 +708,9 @@ def get_warning_str_prefix(tv: str) -> str:
def get_notice_str_prefix(tv: str) -> str:
return ''

def log_usage_warning(self, tv: str, location: T.Optional['mparser.BaseNode']) -> None:
def log_usage_warning(self, tv: str, location: T.Optional['mparser.BaseNode'],
always: bool = False) -> None:
assert not always, "Shouldn't be used with FeatureNew, only FeatureDeprecated"
args = [
'Project targets', f"'{tv}'",
'but uses feature introduced in',
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


class FeatureDeprecated(FeatureCheckBase):
"""Checks for deprecated features"""
Expand All @@ -726,16 +743,26 @@ def get_warning_str_prefix(tv: str) -> str:
def get_notice_str_prefix(tv: str) -> str:
return 'Future-deprecated features used:'

def log_usage_warning(self, tv: str, location: T.Optional['mparser.BaseNode']) -> None:
args = [
'Project targets', f"'{tv}'",
'but uses feature deprecated since',
f"'{self.feature_version}':",
f'{self.feature_name}.',
]
def log_usage_warning(self, tv: str, location: T.Optional['mparser.BaseNode'],
always: bool = False) -> None:
if always:
args = [
'Project uses feature',
self.feature_name,
'which was deprecated in',
f"'{self.feature_version}',",
'and can safely be replaced without causing a regression.'
]
else:
args = [
'Project targets', f"'{tv}'",
'but uses feature deprecated since',
f"'{self.feature_version}':",
f'{self.feature_name}.',
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if self.extra_message:
args.append(self.extra_message)
mlog.warning(*args, location=location)
mlog.warning(*args, location=location, fatal=not always)


# This cannot be a dataclass due to https://github.com/python/mypy/issues/5374
Expand Down