search

archlinuxcn / lilac

Lilac is the build bot for archlinuxcn
GNU General Public License v3.0
114 stars 40 forks source link

The `vcs` nvchecker plugin uses no-longer existent "oldver" #163

Closed yan12125 closed 4 years ago

yan12125 commented 4 years ago

https://github.com/archlinuxcn/lilac/blob/054735691b444c784a18b45333ad9516b45a7f3d/nvchecker_source/vcs.py#L30

conf['oldver'] is no longer set in nvchecker 2.x [1]. As a result, version strings from vcs.py always start with 1.1. Adding oldver back in nvchecker brings back the old behavior, while I suggest to drop the following counter incrementing logic instead and keep using the current 1.1 behavior. The only issue is that version strings will no longer be monotonically increasing, which is not an issue for lilac IMO.

https://github.com/archlinuxcn/lilac/blob/054735691b444c784a18b45333ad9516b45a7f3d/nvchecker_source/vcs.py#L14-L25

[1] https://github.com/lilydjwg/nvchecker/commit/14b3863f11fe72f0cd753a6724c093d1661dfc00#diff-f5ea31dff40b5734ef7f8aac20aa2e8dL178

/cc @yuyichao, author of https://github.com/lilydjwg/nvchecker/pull/1

lilydjwg commented 4 years ago

This option never existed?

yan12125 commented 4 years ago

It does exist in nvchecker 1.x. As a record, it is added just before yuyichao's PR

https://github.com/lilydjwg/nvchecker/commits/5457360858d0b6e8a51bcce4e55b9628684553fa

Or do you mean this option never existed on the document? Then I guess you're right.

lilydjwg commented 4 years ago

Oh yeah, it did exist but didn't get into the docs. I don't remember about it....

@yuyichao requested it so that vcs can compare versions itself. Is this still necessary?

yuyichao commented 4 years ago

I completely forgot that I added vcs support = = ... It seems that the purpose of it is to just make sure the version is always increasing? I don't remember if it was necessary to signal an update then but if it's not necessary now then just returning the output directly should be good enough?

yan12125 commented 4 years ago

just returning the output directly should be good enough?

Not sure about general nvchecker use cases, but the statement should hold for lilac. Given that the vcs plugin is no longer distributed with the main nvchecker package, I would say the move is safe.