<p>Em 28/07/2011 16:10, "Kester Habermann" <<a href="mailto:kester@opencsw.org">kester@opencsw.org</a>> escreveu:<br>
><br>
> Hi Maciej,<br>
><br>
> On Thu, Jul 28, 2011 at 12:50:57PM +0100, Maciej Blizi??ski wrote:<br>
> > Some comments on the code, below.<br>
><br>
> Thanks for the comments. I hadn't even submitted this package yet.<br>
><br>
> > > dsh: created package. libdshconfig: created package<br>
> ><br>
> > It's better for reviews and change tracking if you keep one commit per<br>
> > package.<br>
><br>
> Normally I'd do that. In this case they are coupled dsh needs libdshconfig<br>
> and the author decided to distribute these as independent archived.</p>
<p>Fair enough, as long as you're aware of the issue. </p>
<p>> > > +# File name regex to get notifications about upstream software releases<br>
> > > +# NOTE: Use this only if the automatic regex creation<br>
> > > +#       does not work for your package<br>
> > > +# UFILES_REGEX = $(NAME)-(\d+(?:\.\d+)*).tar.gz<br>
> ><br>
> > You can remove the unused lines.<br>
><br>
> I just left the file as it was created with "gmake newpkg-NAME". You're right<br>
> of course.<br>
><br>
> > > +# for libintl_gettext, libintl_textdomain, libintl_bindtextdomain<br>
> > > +LIBS += -lintl<br>
> > > +export LIBS<br>
> ><br>
> > It's better to let gar handle this by setting EXTRA_LIBS (iirc). Ignition<br>
> > doesn't work in this case, you can make a note why the LIBS variable is<br>
> > exported directly.<br>
><br>
> I looked at<br>
> EXTRA_LIB<br>
>    A list of space separated, additional directories that are passed to the compiler for linking purposes (via the -L and -R flags for Sun Studio).<br>
><br>
> but this takes directories and doesn't work.</p>
<p>There was one that dealt with linker flags, Dago would know. Try to grep for lintl, there must be an example out there.</p>
<p>The idea is that it's better if gar understands the intent.</p>
<p>> I also tried EXTRA_SOS_LD_OPTIONS, but this put the library in the middle and<br>
> not at the end which did not work.</p>
<p>Looks like a pretty demanding build.</p>
<p>> I don't know what you mean with "Ignition".</p>
<p>Sorry about that, it's the android autocompletion and me making a typo. </p>
<p>> The other tricky thing with building this package on the build system is that<br>
> the first package has to be built and installed before the second package may<br>
> be built.</p>
<p>Publishing to unstable makes it relatively easy. The step you can't do yourself is installing the package on the unstable{9,10}{s,x} hosts.</p>
<p>> There is another issue, that there is a conflct with the package CSWclusterit<br>
> which contains a file of the same name (/opt/csw/bin/dsh) which is a completely<br>
> different program. I assume I will have to add that package as conflicting<br>
> package.</p>
<p>In general, we try to avoid creating conflicting packages unless necessary. In this case, CSWclusterit and your package could use alternatives to work around the problem without creating incompatible packages.</p>
<p>> So this is considered work in progress. I submitted it, so it doesn't get lost.</p>
<p>Excellent, it is a good practice. I prefer looking at incremental changes rather than one giant change with everything.</p>
<p>I hope you don't mind me reading and commenting on the code as it flows in.</p>
<p>Maciej</p>