[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#793876: RFS: chrony/1.31.1-1



Le jeu. 20 août 2015 à 12:10, Paul Gevers <elbrus@debian.org> a écrit :
Hi Vincent,

Hello Paul,

Live from Debconf15.

I’m watching you folks, you all look great. :-)

On 19-08-15 21:29, Vincent Blut wrote:
 I am looking for a sponsor for my package "chrony"

Please note this is a first manual inspection. Not all items are
critical, most are just nitpicks or tips or questions.

Please add the CVE numbers that were fixed by upstream to your changelog such that the trackers can find it automatically. TIP: if you would have
done that and mention that in your RFS you would have probably found a
sponsor earlier.

I didn’t include them because those fixes have been backported to chrony 1.30-2 in Debian (thanks Joachim btw) and consequently the CVE numbers have
already been mentionned in this release’s changelog.

Your priority switch from extra to optional may require a ping to
somebody. I am not sure and I would need to search, so please do that
yourself.

Yes. I’ll have to send a bug report to the ftp.debian.org pseudo-package asking for the modification of the section/priority in the "override-file".
Will do later today… or tomorrow.

Which file do you have in common with ntp? Please re-read ¹.

I guess I’ve been misled by § 7.6.2! The previous section shows the usage
of the 'replaces:' field for packages providing *files* already provided
by another package. However, the section 7.6.2 seems to be for *packages* that /do conflict/; I interpreted that /do conflict/ by "packages providing the same functionality". I even was quite sure my interpretation was correct after seeing
the usage example about MTAs.

Anyway, depending on your answer, I’ll revert this commit.

I assume that the change of maintainership has the consent of Joachim?

Yes, we’ve discussed about this privately some times ago. Still ok Joachim?

Wouldn't the hwclockfile stuff in /etc not warrant an debian/NEWS
update? Or at the very least some help in the changelog?

I don’t think so. Finally, that change have no impact for the user.
Previously we had to check (in the postinst script) if the RTC keeps local time or UTC by parsing /etc/adjtime and/or /etc/default/rcS. Depending on the result,
we set (or no) the "rtconutc" directive in /etc/chrony.conf.
But now chrony is grown enough to check that by itself. Each time it is started,
it will parse the correspondent value in the /etc/adjtime file.

So, as you can see, the whole point of using the hwclockfile directive is to have something cleaner than playing the text processing game for the same result.

 Doesn't this
actually require a migration path? What if the /etc/chrony and
/etc/adjtime are NOT answering the same?

Well, I’m not sure I’m understanding you here. The chronyd daemon will use what /etc/adjtime returns, thanks to the 'hwclockfile /etc/adjtime' directive.

Can you please explain me how commit 1ce86d3 works (the Breaks of
util-linux).

As the hwclockfile directive can only deal with /etc/adjtime, we need to ensure that we migrated from /etc/default/rcS to /etc/adjtime. To be honest, I’m not sure that break is even needed, because this migration happened prior to Wheezy.

I assume you tested all migrations for admins that already ran chrony as
a different users as described in the README.Debian. Are the manual
steps even needed? Shouldn't this go into a NEWS file instead of the
README file?

I tested a lot of use cases, but Jerome Benoit informed me he had an issue possibly related to this change, but as he uses a custom init script etc., I will have to
check his atypical configuration.

Line 36 of the README.Debian file ends weird now, you removed a filename
but not the "and" in front.

Indeed, will fix. You mean line 27 right?

Nice to have, could you think of some autopkgtest test²? And why are the
tests disabled. Unless they fail and can't be fixed, it is really
recommended to run them.

I’m definitely interested in autopkgtest. However I’ll need some times to dive
through its meanders.
I don’t known why tests have originally been disabled, but I guess it’s because they depend on the clknetsim tool which is not packaged in Debian. Also, if that tool isn’t installed on the system, the "test.common" script will try to download and build the tool, which is a quite invasive method. Am I wrong Joachim?

I think the comments you added in commit df80cd25 in the copyright file,
should the "Comment" field.³

Definitely, will fix.

And tip to prevent the fix in commit 7245a4, use dch to write the
timestamps (e.g. dch -rm)

So true. I discovered this tool to late. ;-)
Thanks for the reminder.

You could maybe remind upstream to update their copyright years when
they make changes.

Indeed.

Paul

Thanks for the initial review,
Vincent


Reply to: