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.

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

incoherent-version-in-changelog is factoring .%dist in %{release} #856

Open
brianjmurrell opened this issue May 5, 2022 · 21 comments
Open

Comments

@brianjmurrell
Copy link

I am getting a W: incoherent-version-in-changelog 1.15.0~rc3-2 ['1.15.0~rc3-2.suse.lp153', '1.15.0~rc3-2.suse.lp153'] warning.

This is due to Release: 2%{?dist} in the specfile. But I cannot include either %{dist} or even %{release} in the changelog version or else I get W: macro-in-%changelog %{release}. I certainly cannot hard code a given distro's %{dist} value either.

It seems like

expected = [version + '-' + release]
if epoch is not None: # regardless of use_epoch
expected[0] = str(epoch) + ':' + expected[0]
# Allow EVR in changelog without release extension,
# the extension is often a macro or otherwise dynamic.
if self.release_ext:
expected.append(self.extension_regex.sub('', expected[0]))
# Check if a package does not have a version that is
# compatible with epoch:vesrion-release tuple
if ret.group(1) not in expected:
if len(expected) == 1:
expected = expected[0]
self.output.add_info('W', pkg, 'incoherent-version-in-changelog',
ret.group(1), expected)
needs to account for a possible .%{?dist} in Release:.

@marxin
Copy link
Contributor

marxin commented May 5, 2022

This is due to Release: 2%{?dist} in the specfile.

It seems strange adding %{dist} to Release.

But I cannot include either %{dist} or even %{release} in the changelog version or else I get W: macro-in-%changelog %{release}. I certainly cannot hard code a given distro's %{dist} value either.

Well, I can imagine supporting any of the 2 macros in %changelog but I would like to see your use-case?

@brianjmurrell
Copy link
Author

It seems strange adding %{dist} to Release.

To just pick a specfile at random:

https://src.fedoraproject.org/rpms/autofs/blob/rawhide/f/autofs.spec#_15

I think you will find that all Fedora, RHEL (and therefore all EL downstreams) and SUSE specfiles do that.

Well, I can imagine supporting any of the 2 macros in %changelog but I would like to see your use-case?

As above? But honestly, I'd rather not use %{release} as it defeats the purpose of the changelog and having to add %{?dist} to every changelog entry just seems silly.

It seems far more useful for expected to just allow for %{version}-%{release} with .%{dist} removed. But yes, how do you know what %{dist} is for a SUSE RPM when running rpmlint on say, a Fedora machine. I can only suppose you need to keep a list of common %{dist}s.

@marxin
Copy link
Contributor

marxin commented May 6, 2022

To just pick a specfile at random:

https://src.fedoraproject.org/rpms/autofs/blob/rawhide/f/autofs.spec#_15

I think you will find that all Fedora, RHEL (and therefore all EL downstreams) and SUSE specfiles do that.

All right, so then let's allow %{dist} in Release then. Do you want to come up with a pull request?

@brianjmurrell
Copy link
Author

Do you want to come up with a pull request?

Unfortunately this is a $day_job issue and I have too many other deadlines looming to have to take on another yak to shave.

But there is a different kind of consistency issue here. Why do I only see this while parsing a suse RPM and not a, say, EL7 or EL8 RPM? They are all built from the same specfile and therefore all have %{?dist} expanded in their Release: but no in their %changelog entries.

What is special about the el7 and el8 %{?dist}s that they don't raise this issue?

@marxin
Copy link
Contributor

marxin commented May 13, 2022

Please upload somewhere RPM files so that I can observe the difference.

@brianjmurrell
Copy link
Author

Hrm. I don't really have anywhere I can upload them to.

If you use mock to build https://github.com/daos-stack/dpdk/blob/1231cac43173877aae6b152ded5523d80809bac9/dpdk.spec on each of opensuse-leap-15.3-x86_64 and rocky+epel-8-x86_64 you will get RPMs you can compare.

@marxin
Copy link
Contributor

marxin commented Jun 15, 2022

Hrm. I don't really have anywhere I can upload them to.

Please zip them and attach them to this Issue.

@Conan-Kudo
Copy link
Member

@brianjmurrell Do you have a COPR project where this was built? That would also work, since they use the same Mock definitions.

@marxin
Copy link
Contributor

marxin commented Sep 5, 2022

Can you please attach the problematic RPM file?

@brianjmurrell
Copy link
Author

@marxin
Copy link
Contributor

marxin commented Sep 5, 2022

Thanks for the file.

So can you please experiment with something like:

diff --git a/rpmlint/checks/SpecCheck.py b/rpmlint/checks/SpecCheck.py
index 997d974c..739840be 100644
--- a/rpmlint/checks/SpecCheck.py
+++ b/rpmlint/checks/SpecCheck.py
@@ -436,7 +436,7 @@ class SpecCheck(AbstractCheck):
                                          '%%changelog: %s' % deptoken)
                 for match in self.macro_regex.findall(line):
                     res = re.match('%+', match)
-                    if len(res.group(0)) % 2 and match != '%autochangelog':
+                    if len(res.group(0)) % 2 and match not in ('%autochangelog', '%release', '%dist'):
                         self.output.add_info('W', pkg, 'macro-in-%changelog', match)
             else:
                 if not depscript_override:

which should not warn about %release nor %dist macros in %changelog.

@Conan-Kudo
Copy link
Member

The %release macro should never be in the changelog, nor should %dist.

@Conan-Kudo
Copy link
Member

Conan-Kudo commented Sep 6, 2022

@brianjmurrell In the case of writing out EVRs in the changelog entry, you should omit the %dist macro, because that value changes depending on who it is built for. So instead of 1.0-1%{?dist}, just write 1.0-1.

Basically, this is not a bug, and this is working as intended.

@brianjmurrell
Copy link
Author

brianjmurrell commented Sep 6, 2022

@Conan-Kudo Indeed, I completely agree. While it's been a while since I filed this ticket, because I generally agree with you I can only imagine that I tried including %{dist} to try to appease some other rpmlint error. I will try to revisit this.

Indeed, if you go re-read the original comment here, I do say:

I cannot include either %{dist} or even %{release} in the changelog version

So I acknowledge that I don't really want to do that,

The title of this issue even is incoherent-version-in-changelog is factoring .%dist in %{release} which is describing that it's not my attempting to include .%{dist} in the %changelog that is at the root of the problem but rather that rpmlint is having a fit because %{dist} is in the Release:, and that is disagreeing with what is in the %changelog entry's version (when it does not try to include .%{dist} in it.

@brianjmurrell
Copy link
Author

if len(res.group(0)) % 2 and match not in ('%autochangelog', '%release', '%dist'):

But per @Conan-Kudo (and I agree with him), I shouldn't actually be putting the %dist macro into the %changelog entry, so this solution seems to simply allow the hacky work-around rather than addressing the root problem of having the %dist in the Release: being factored into the comparison of the version in the %changelog.

That said, the work-around does work. It just doesn't seem to be the correct solution where the correct solution, I would think, is to disable the factoring of %dist into Release: when comparing the version with the one in the %changelog.

@brianjmurrell
Copy link
Author

Any further thoughts given the last couple of comments?

@marxin
Copy link
Contributor

marxin commented Sep 26, 2022

Let's use the workaround for now. The issue is correct, but has very low priority for me, sorry.

@brianjmurrell
Copy link
Author

What is the workaround exactly?

@marxin
Copy link
Contributor

marxin commented Sep 27, 2022

The one mentioned in #856 (comment) ?

@brianjmurrell
Copy link
Author

Do you mean using %{dist} in the %changelog entry? It seems count-productive to break the data just to appease a broken validation tool.

I think I'd sooner add an ignore in the rpmlintrc than to start breaking the real-world data just to appease the broken validator.

So I guess that is what I will do until this issue is fixed.

Thanks for all of the input.

@marxin
Copy link
Contributor

marxin commented Sep 27, 2022

I think I'd sooner add an ignore in the rpmlintrc than to start breaking the real-world data just to appease the broken validator.

Yes, that would be the easiest approach.

Thanks for the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants