rpm-software-management / spec-cleaner

spec-cleaner
BSD 3-Clause "New" or "Revised" License
27 stars 33 forks source link

http -> https rewrite doesn't work when rpmspec call fails #304

Closed kstreitova closed 2 months ago

kstreitova commented 1 year ago

With #300 we started to use rpmspec to expand Source addresses and check if we could switch to https. However, if the rpmspec call fails, it doesn't parse the specfile and thus the replacement doesn't happen. And it's not hard to make it fail, it's enough that one of the sections of the specfile is missing (try to remove e.g. %description to test it).

@jsegitz We probably need to find a different way to expand the Source line.

jsegitz commented 1 year ago

This is an opportunistic hardenning, so I don't have big issues with this failing on occasion. But I'm open to do this differently, but ATM I don't see a good alternative to rpmspec. The source addresses need to be expanded and they can contain arbitrary constructs, so doing this in spec-cleaner directly would be hard and also error prone

kstreitova commented 1 year ago

The problem is primarily tests. We often use snippets of specfiles for testing and right now, out of ~100 test files only 1 is not failing with rpmspec -P.

Most of the time, the reason is missing a section or {Name,Version, Release, Summary,License,...} field, but often it's also:

This makes using rpmspec not reliable, mainly in our test suite.

Maybe we can use some Python rpmspec parser library here:

jsegitz commented 1 year ago

ah okay. That is unfortunate. I'll have a look, but it will be a while. Maybe it's best to remove this commit until this is fixed, I don't want to break your tests until I get to this

kstreitova commented 1 year ago

It's not super urgent, the tests are passing right now (mostly because Sources don't have https version so there is no replacement happening and it's passing "by accident"). But I can imagine that in the future it can be confusing when people add new tests with an "incomplete" specfile and http -> https replacement won't work as they expect.

jsegitz commented 1 year ago

okay, good to know. I'll revisit this a bit later then, currently I'm swamped