Bug 497756 - Review Request: lpg - LALR Parser Generator
Review Request: lpg - LALR Parser Generator
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Andrew Overholt
Fedora Extras Quality Assurance
:
: 486365 (view as bug list)
Depends On:
Blocks: 485460
  Show dependency treegraph
 
Reported: 2009-04-26 18:37 EDT by Mat Booth
Modified: 2009-07-15 07:05 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-15 07:05:10 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
overholt: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Mat Booth 2009-04-26 18:37:35 EDT
Spec URL: http://mbooth.fedorapeople.org/reviews/lpg.spec
SRPM URL: http://mbooth.fedorapeople.org/reviews/lpg-1.1.0-1.fc10.src.rpm
Description:
The LALR Parser Generator (LPG) is a tool for developing scanners and parsers
written in Java, C++ or C. Input is specified by BNF rules. LPG supports
backtracking (to resolve ambiguity), automatic AST generation and grammar
inheritance.

Packaging notes:

- This package is needed to build outstanding parts of eclipse-dtp that we don't yet build.
- This is a reasonably ancient version of the package because of eclipse-dtp and unfortunately later versions of LGP have incompatible APIs.
Comment 1 Igor Jurišković 2009-04-26 19:37:24 EDT
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
Comment 2 manuel wolfshant 2009-04-27 00:55:33 EDT
(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.
Comment 3 Igor Jurišković 2009-04-27 06:34:35 EDT
(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.
Comment 4 Andrew Overholt 2009-04-27 09:44:52 EDT
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
Comment 5 Andrew Overholt 2009-04-27 09:52:44 EDT
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
Comment 6 Andrew Overholt 2009-04-27 10:02:23 EDT
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?
Comment 7 Andrew Overholt 2009-04-27 10:05:15 EDT
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.
Comment 8 Mat Booth 2009-04-28 07:33:32 EDT
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.
Comment 9 Mat Booth 2009-04-28 08:02:24 EDT
(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.
Comment 10 Andrew Overholt 2009-04-28 08:58:28 EDT
(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
Comment 12 Andrew Overholt 2009-05-01 12:33:28 EDT
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.
Comment 13 Andrew Overholt 2009-05-12 15:08:12 EDT
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.
Comment 14 Andrew Overholt 2009-05-19 09:31:28 EDT
*** Bug 486365 has been marked as a duplicate of this bug. ***
Comment 15 Mat Booth 2009-05-19 17:29:08 EDT
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.
Comment 16 Mat Booth 2009-06-20 12:10:07 EDT
Any progress on this?
Comment 17 Andrew Overholt 2009-06-22 10:24:13 EDT
Apologies, Mat, I haven't had a chance to look at this.  I'll try to do it this week.
Comment 18 Mat Booth 2009-06-22 11:54:45 EDT
(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. :-)
Comment 19 Mat Booth 2009-07-04 12:32:33 EDT
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
Comment 20 Mat Booth 2009-07-05 15:47:25 EDT
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
Comment 21 Mat Booth 2009-07-07 14:15:08 EDT
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!
Comment 22 Andrew Overholt 2009-07-13 09:42:37 EDT
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.
Comment 23 Mat Booth 2009-07-13 15:51:45 EDT
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.
Comment 24 Andrew Overholt 2009-07-13 17:34:53 EDT
Do it up!  Thanks again :)
Comment 25 Mat Booth 2009-07-14 04:32:55 EDT
New Package CVS Request
=======================
Package Name: lpg
Short Description: LALR Parser Generator
Owners: mbooth
Branches: F-11
Comment 26 Jason Tibbitts 2009-07-14 22:30:59 EDT
CVS done.
Comment 27 Mat Booth 2009-07-15 07:05:10 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.