| Message ID | 1367856775-4502-1-git-send-email-ntl@pobox.com |
|---|---|
| State | Superseded |
| Headers | show |
Dear Nathan Lynch, Thanks, this looks almost good. A few comments below. On Mon, 6 May 2013 11:12:55 -0500, Nathan Lynch wrote: > diff --git a/package/chrony/Config.in b/package/chrony/Config.in > new file mode 100644 > index 0000000..a42bd77 > --- /dev/null > +++ b/package/chrony/Config.in > @@ -0,0 +1,5 @@ > +config BR2_PACKAGE_CHRONY > + bool "chrony" > + help > + Chrony is a pair of programs which are used to maintain the > + accuracy of the system clock. We generally want a empty new line here, and then the upstream URL of the project. Also, did you check that this program indeed builds with a minimal uClibc toolchain (no thread, no wchar, no locale, no nothing) ? > diff --git a/package/chrony/chrony.mk b/package/chrony/chrony.mk > new file mode 100644 > index 0000000..b9733f9 > --- /dev/null > +++ b/package/chrony/chrony.mk > @@ -0,0 +1,6 @@ We generally like to have our nice header here, like in all other packages: ############################################################# # # bind # ############################################################# > +CHRONY_VERSION = 1.27 > +CHRONY_SITE = http://download.tuxfamily.org/chrony/ > +CHRONY_LICENSE = GPLv2 > +CHRONY_LICENSE_FILES = COPYING > + > +$(eval $(autotools-package)) Looks good. Thanks! Thomas
On Mon, 2013-05-06 at 19:20 +0200, Thomas Petazzoni wrote: > Dear Nathan Lynch, > > Thanks, this looks almost good. A few comments below. > > On Mon, 6 May 2013 11:12:55 -0500, Nathan Lynch wrote: > > > diff --git a/package/chrony/Config.in b/package/chrony/Config.in > > new file mode 100644 > > index 0000000..a42bd77 > > --- /dev/null > > +++ b/package/chrony/Config.in > > @@ -0,0 +1,5 @@ > > +config BR2_PACKAGE_CHRONY > > + bool "chrony" > > + help > > + Chrony is a pair of programs which are used to maintain the > > + accuracy of the system clock. > > We generally want a empty new line here, and then the upstream URL of > the project. Okay, will fix. > Also, did you check that this program indeed builds with a minimal > uClibc toolchain (no thread, no wchar, no locale, no nothing) ? Sorry, no, that was sloppy of me. I will do so for v2 and try to better specify the package's needs and dependencies. I'd be happy to try any configs you'd care to point out... > > diff --git a/package/chrony/chrony.mk b/package/chrony/chrony.mk > > new file mode 100644 > > index 0000000..b9733f9 > > --- /dev/null > > +++ b/package/chrony/chrony.mk > > @@ -0,0 +1,6 @@ > > We generally like to have our nice header here, like in all other > packages: > > ############################################################# > # > # bind > # > ############################################################# Will fix. Thanks for the review, Thomas.
Dear Nathan Lynch, On Mon, 06 May 2013 12:42:36 -0500, Nathan Lynch wrote: > > Also, did you check that this program indeed builds with a minimal > > uClibc toolchain (no thread, no wchar, no locale, no nothing) ? > > Sorry, no, that was sloppy of me. I will do so for v2 and try to > better specify the package's needs and dependencies. I'd be happy to > try any configs you'd care to point out... You can try a very minimal build: no thread, no wchar, no locale, no C++, no nothing. If it builds, you're all set. If your package doesn't build, then something is missing. If you don't want to spend too much time building toolchains, you can use: http://autobuild.buildroot.org/toolchains/tarballs/br-arm-basic.tar.bz2 as an external toolchain that is minimal (no C++, no wchar, no locale, no largefile, etc. but it has thread support) http://autobuild.buildroot.org/toolchains/tarballs/br-arm-full-nothread.tar.bz2 as an external that has everything (C++, wchar, locale, largefile, etc.), but doesn't has thread support. If your package builds with both, then normally you should be all set. And anyway, since those toolchains are the ones used on the autobuilders, if your package doesn't build in some configuration, it won't be catched by the autobuilders anyway :-) Best regards, Thomas
On Mon, 2013-05-06 at 19:49 +0200, Thomas Petazzoni wrote: > Dear Nathan Lynch, > > On Mon, 06 May 2013 12:42:36 -0500, Nathan Lynch wrote: > > > > Also, did you check that this program indeed builds with a minimal > > > uClibc toolchain (no thread, no wchar, no locale, no nothing) ? > > > > Sorry, no, that was sloppy of me. I will do so for v2 and try to > > better specify the package's needs and dependencies. I'd be happy to > > try any configs you'd care to point out... > > You can try a very minimal build: no thread, no wchar, no locale, no > C++, no nothing. If it builds, you're all set. If your package doesn't > build, then something is missing. > > If you don't want to spend too much time building toolchains, you can > use: > > http://autobuild.buildroot.org/toolchains/tarballs/br-arm-basic.tar.bz2 > as an external toolchain that is minimal (no C++, no wchar, no locale, > no largefile, etc. but it has thread support) > > http://autobuild.buildroot.org/toolchains/tarballs/br-arm-full-nothread.tar.bz2 > as an external that has everything (C++, wchar, locale, largefile, > etc.), but doesn't has thread support. > > If your package builds with both, then normally you should be all set. Thank you for this guidance. I verified that the package builds with both of these and found no issues (I did realize it needs MMU, however). I will do similarly for future package submissions. v2 with the rest of your feedback addressed on its way.
Dear Nathan Lynch, On Mon, 06 May 2013 14:01:52 -0500, Nathan Lynch wrote: > Thank you for this guidance. I verified that the package builds with > both of these and found no issues (I did realize it needs MMU, > however). I will do similarly for future package submissions. > > v2 with the rest of your feedback addressed on its way. Thanks a lot! Thomas
diff --git a/package/Config.in b/package/Config.in index fe4f56b..d5719a8 100644 --- a/package/Config.in +++ b/package/Config.in @@ -670,6 +670,7 @@ source "package/bind/Config.in" source "package/bmon/Config.in" source "package/bridge-utils/Config.in" source "package/can-utils/Config.in" +source "package/chrony/Config.in" source "package/connman/Config.in" source "package/crda/Config.in" source "package/ctorrent/Config.in" diff --git a/package/chrony/Config.in b/package/chrony/Config.in new file mode 100644 index 0000000..a42bd77 --- /dev/null +++ b/package/chrony/Config.in @@ -0,0 +1,5 @@ +config BR2_PACKAGE_CHRONY + bool "chrony" + help + Chrony is a pair of programs which are used to maintain the + accuracy of the system clock. diff --git a/package/chrony/chrony.mk b/package/chrony/chrony.mk new file mode 100644 index 0000000..b9733f9 --- /dev/null +++ b/package/chrony/chrony.mk @@ -0,0 +1,6 @@ +CHRONY_VERSION = 1.27 +CHRONY_SITE = http://download.tuxfamily.org/chrony/ +CHRONY_LICENSE = GPLv2 +CHRONY_LICENSE_FILES = COPYING + +$(eval $(autotools-package))
Signed-off-by: Nathan Lynch <ntl@pobox.com> --- package/Config.in | 1 + package/chrony/Config.in | 5 +++++ package/chrony/chrony.mk | 6 ++++++ 3 files changed, 12 insertions(+) create mode 100644 package/chrony/Config.in create mode 100644 package/chrony/chrony.mk