Bug 497756

Hi Mat,

first of all this is not official review. I'm in need of sponsor.

URL:       http://lpg.sourceforge.net/
It is much better to use macro. If the package name changes you will not need to change the url too.
URL:       http://%{name}.sourceforge.net/

You are using cp to install files. You should use install. Like:
install -pD %{name}javaruntime.jar %{buildroot}%{_javadir}/%{name}javaruntime-%{version}.jar

When doing symlinks you don't need to change directory. You can do it this way:
ln -s ../%{javadir}/%{name}javaruntime-%{version}.jar %{buildroot}%{_javadir}/%{name}-%{version}.jar

%description manual 
I wouldn't use that small description because many don't now what LPG is. You could use description of LPG then simply add at the end "This is programmers manual for LPG."

Why are you naming package lpg then later using name lpgdistribution? You should use the real name of library as the name of the package. If you have good reason for not doing so comment the spec file.

You are packaging manual files as different package. Why? There are only 2 files. If they are large(definition of large is left to you) package them as %{name}-doc

(In reply to comment #1)
> Hi Mat,
> 
> first of all this is not official review. I'm in need of sponsor.
> 
> URL:       http://lpg.sourceforge.net/
> It is much better to use macro. If the package name changes you will not need
> to change the url too.
> URL:       http://%{name}.sourceforge.net/
But on the other hand
- names of the projects very rarely change
- using macros in the URL / SOURCE tags block usage of certain automated verification tools. And even simple copy / paste does not work any more.
So I beg to differ, but using macros here is not at all much better.

(In reply to comment #2)
> (In reply to comment #1)
> > Hi Mat,
> > 
> > first of all this is not official review. I'm in need of sponsor.
> > 
> > URL:       http://lpg.sourceforge.net/
> > It is much better to use macro. If the package name changes you will not need
> > to change the url too.
> > URL:       http://%{name}.sourceforge.net/
> But on the other hand
> - names of the projects very rarely change
> - using macros in the URL / SOURCE tags block usage of certain automated
> verification tools. And even simple copy / paste does not work any more.
> So I beg to differ, but using macros here is not at all much better.  

You're right. I used macro in my review request and nobody complained. I guess both ways are right. You could wait for advanced packager to tell you what is better.

I'll take this one.  It sucks that DTP needs such an old version of LPG.  I wonder if that's because this version is the one included in Orbit [1]?  If so, perhaps we can work with the DTP people -- CDT uses lpg, too -- to update their API usage.

Full review forthcoming.

[1]
http://eclipse.org/orbit

Mat, are there C/C++ bits we're not building?  If so, we should probably either build them (and make it not noarch, etc.) or remove the comments from the description about "... C, C++ ..." and rename to lpgjavaruntime.  What do you think?  Also, we'll need OSGi metadata for DTP, right?  And I don't think we should be injecting the license if upstream doesn't include it (see [1]).

I'll post a more formal review in a bit.  Thanks for submitting this!

[1]
http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

Actually, I'm going to hold off on an full review until we decide whether or not to build the native bits.  I know it won't help the DTP situation, but I'm thinking we should build all of the latest release -- 2.0.16 -- and help DTP to update their usage to the new API.  What do you think, Mat?

Alternatively, we could just build the latest lpg-java-runtime and call the package that.  Then we don't have to worry about the C/C++ stuff.  They are distributed separately on SourceForge so I don't think that would be too crazy.

Holy crap, comments!

Andrew, I especially like how you compose all of your thoughts into a single coherent comment. ;-)

I agree we should build the latest LPG (and adding the native bits will be trivial), but porting DTP/CDT isn't something I'd be confident doing or have the time to commit to.

However, the jar and Java package names changed between LPG 1 and LPG 2 so it would be possible to package both jars without conflict until such time as the LPG 1 jar can be obsoleted. So I propose the following packages:

  lpg: the main package containing the tool to generate grammar parsers (this is the native C++ bit)

  lpg-java: subpackage containing the java runtime library for java parsers generated by the tool in the main package (lpgruntime.jar)

  lpg-java-compat: subpackage containing the old java runtime library (lpgjavaruntime.jar)

  lpg-manual: programmer/user guides for both lpg and lpg-java

lpg and lpg-java could be combined, of course, but DTP's dependency is only on the Java portion -- just depends on how much you care about that sort of thing.

I can knock up such a package this evening if you approve. I will also fix my licence injection (a copy of the licence is packaged with the newest version) and fix my use of cp to install files (thanks Igor).

Igor: The *-manual subpackage is just convention for java packages. API documentation goes in *-javadoc and other literature goes in *-manual.

(In reply to comment #1)
> Why are you naming package lpg then later using name lpgdistribution?

When extracting source archives, %setup assumes by default that the top-level directory will be called %{name}-%{version}, which wasn't true in this case. This is why it was necessary to use the -n option to tell setup the name of the top-level directory inside the archive.

(In reply to comment #8)
> Andrew, I especially like how you compose all of your thoughts into a single
> coherent comment. ;-)

:P

> I agree we should build the latest LPG (and adding the native bits will be
> trivial), but porting DTP/CDT isn't something I'd be confident doing or have
> the time to commit to.

I can take a look at porting DTP.  CDT is less of a big deal since we're not using this functionality now.  I think it's their c99 parser that uses LPG.

> So I propose the following packages:
> 
>   lpg: the main package containing the tool to generate grammar parsers (this
> is the native C++ bit)
> 
>   lpg-java: subpackage containing the java runtime library for java parsers
> generated by the tool in the main package (lpgruntime.jar)
> 
>   lpg-java-compat: subpackage containing the old java runtime library
> (lpgjavaruntime.jar)
> 
>   lpg-manual: programmer/user guides for both lpg and lpg-java

This sounds good to me.  It doesn't look like this is done yet, but soon you'll be able to have lpg-java be a noarch sub-package and the C++ bits be arch-specific:

http://fedoraproject.org/wiki/Features/NoarchSubpackages

How about this:

Spec URL: http://mbooth.fedorapeople.org/reviews/lpg.spec
SRPM URL: http://mbooth.fedorapeople.org/reviews/lpg-2.0.16-1.fc10.src.rpm

Here's a mostly-complete review with a few questions:

- naming and licensing are fine
- versioning is fine
- where does Source3 (the build.xml) come from?  what about Patch1 (-jar-up)?
- I didn't know you could use the %setup macro more than once but I don't see anything in the guidelines about not doing this
- a comment other than "apply patches" would be useful before the %patch macros.  I personally like to say what each patch is doing
- lines 16, 99, 101, 108, 109, 110 are all > 80 characters
- the Makefile(s) they provide do something weird with debuginfo ... or /usr/bin/lpg isn't executable so strip-debuginfo isn't run on it?

I have to go over the non-Java packaging guidelines for the C++ stuff.  That'll take me a while but I'll get back to you when I'm done.

I can easily take the Java side of things but I'd appreciate Will's help with the non-Java stuff.  Will, would you mind reviewing the C++ stuff?  Thanks.

*** Bug 486365 has been marked as a duplicate of this bug. ***

Sorry for the silence on this front, busy couple of weeks.

It's no bother calling the %setup macro more than once (most useful for packages with multiple source archives). See: http://www.rpm.org/max-rpm/s1-rpm-inside-macros.html

I've better documented the source files and patches and added some line continuations:

Spec URL: http://mbooth.fedorapeople.org/reviews/lpg.spec
SRPM URL: http://mbooth.fedorapeople.org/reviews/lpg-2.0.16-2.fc10.src.rpm

---

Also, I'm not sure what you're see with regards to debuginfo but in my local build (on F10) I see this come out just before it starts processing dependencies:

+ /usr/lib/rpm/find-debuginfo.sh --strict-build-id /home/mbooth/rpmbuild/BUILD/lpg-1.1.0
extracting debug info from /home/mbooth/rpmbuild/BUILDROOT/lpg-2.0.16-1.fc10.i386/usr/bin/lpg
2941 blocks

And it's definitely building a debuginfo package with some stuff inside. However, I wouldn't know a useful debuginfo package if it came up and shook my hand; it's not something I know very much about.

Any progress on this?

Apologies, Mat, I haven't had a chance to look at this.  I'll try to do it this week.

(In reply to comment #17)
> Apologies, Mat, I haven't had a chance to look at this.  I'll try to do it this
> week.  

No biggie, just pinging. :-)

Bit of an update:

- Update to 2.0.17.
- Add OSGI manifest info to the runtime jar.
- Bundle generator docs with the generator in the main package.


Spec URL: http://mbooth.fedorapeople.org/reviews/lpg.spec
SRPM URL: http://mbooth.fedorapeople.org/reviews/lpg-2.0.17-1.fc10.src.rpm

And again:

- Fix version number on the java libraries.

Spec URL: http://mbooth.fedorapeople.org/reviews/lpg.spec
SRPM URL: http://mbooth.fedorapeople.org/reviews/lpg-2.0.17-2.fc10.src.rpm

Just FYI, I've now got an update to eclipse-dtp that builds all the features except for the SDK ready to go for when this package gets approved.

To try it out you will need to build and install lpg-java-compat (a subpackage of LPG) on a Rawhide box (an update is needed to wsdl4j on F11). Here is the new eclipse-dtp srpm (I've not booked this into CVS yet):

http://mbooth.fedorapeople.org/eclipse-dtp-1.7.0-2.fc12.src.rpm

At last, decent SQL editors in Eclipse!

Thanks for untangling this mess, Mat!  The package looks great.  Hopefully we can port over dependencies to 2.x APIs and get rid of the compat stuff soon.  As for review points, everything looks good to me:

* naming and versioning good
* licensing okay
* md5sums match of my downloads and the SRPM-included ones
* patches seem okay (would be nice to get upstream to include build.xml but the OSGi stuff is probably not something they'll take)
* files okay
* rpmlint silent on SRPM and RPMs
* no pre-built binaries included (except the JAR which gets re-generated so it's okay)
* macros good
* Requires/BuildRequires seem fine
* debuginfo is non-zero so I'll assume it's generated okay :)
* no shared libraries or static libraries
* no rpath usage
* no config files
* doc files and license files good
* no .desktop file
* no lang packs
* nice use of global over define
? should %{?_smp_mflags} be passed to make?
* not relocatable
* file ownership seems fine
* not a web app
* links to upstream bugs and comments -- good

My only concern is the large number of compiler warnings in the C++ generator stuff.  Do you have any thoughts on these?

I apologize for the incredible delay on my review.  This package is good to go.

Thanks for the review.

(In reply to comment #22)
> Thanks for untangling this mess, Mat!  The package looks great.  Hopefully we
> can port over dependencies to 2.x APIs and get rid of the compat stuff soon. 
> As for review points, everything looks good to me:
> 
> ? should %{?_smp_mflags} be passed to make?

When I add the smp flags, it deletes the object files before it links the executable. I'm not really versed well enough with Makefiles to know why and it's not a massive build so I wasn't convinced it was worth the effort to figure it out. (Any suggestions welcome, though.) See this build log snippet:

  rm -f core ,* *~ *.bak
  rm -f *.o gmon.out mon.out TAGS tags
  In file included from util.h:4,
                   from option.h:4,
                   from control.h:4,
                   from scanner.cpp:2:
  tuple.h: In member function 'T Stack<T>::Pop() [with T = char*]':
  tuple.h:273: warning: assuming signed overflow does not occur when assuming that (X - c) > X is always false
  tuple.h: In member function 'char* Scanner::ScanOptions()':
  tuple.h:273: warning: assuming signed overflow does not occur when assuming that (X - c) > X is always false
  tuple.h:273: warning: assuming signed overflow does not occur when assuming that (X - c) > X is always false
  g++ CAction.o CTable.o CppAction.o CppTable.o JavaAction.o JavaTable.o LexStream.o MlAction.o MlTable.o PlxAction.o PlxTable.o PlxasmAction.o PlxasmTable.o XmlAction.o XmlTable.o Action.o base.o buffer.o code.o control.o dfa.o diagnose.o dump.o generator.o grammar.o jikespg.o jikespg_act.o jikespg_init.o jikespg_prs.o option.o parser.o pda.o produce.o resolve.o scanner.o sp.o symbol.o tab.o table.o util.o -o lpg-linux_x86
  g++: CAction.o: No such file or directory
  g++: CTable.o: No such file or directory
  etc...
  etc...

(In reply to comment #22)
> My only concern is the large number of compiler warnings in the C++ generator
> stuff.  Do you have any thoughts on these?

About 99% of these are an ignored return value from fwrite in buffer.h that just about every other source file includes, so it looks to me like it's just the same warning over and over. (I guess it just doesn't care about how many bytes it has written.)

(In reply to comment #22)
> I apologize for the incredible delay on my review.  This package is good to go.  

No probs. If you're happy enough with my comments above I'll get it booked into CVS.

Do it up!  Thanks again :)

New Package CVS Request
=======================
Package Name: lpg
Short Description: LALR Parser Generator
Owners: mbooth
Branches: F-11

CVS done.

I had to make one minor change after booking it in (turns out I missed a BuildReq on ant-apache-regexp) but it's now built for all requested branches. Closing the ticket.