[Bug 244192] Review Request: eclipse-anyedit - AnyEdit plugin for Eclipse

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: eclipse-anyedit - AnyEdit plugin for Eclipse


https://bugzilla.redhat.com/show_bug.cgi?id=244192





------- Additional Comments From rob.myers at gtri.gatech.edu  2007-09-13 09:20 EST -------
(In reply to comment #8)
> Hi Rob,
> 
> I have just finish the pre-review of this package - In the big part the package
> seems to be OK, there is just two things that need to be fixed. I have attached
> a patch that fix these two issue.

the patch seems to reflect your packaging preferences rather than Fedora
packaging requirements.  for example, the patch changes a line that is
cut'n'pasted directly from http://fedoraproject.org/wiki/NativeJava #6.

we should probably work with Ben Konrath and Andrew Overholt to come up with an
EclipseAddons packaging guide- that way we can capture the best practices and
ensure some level of consistency between the eclipse plugin packages.

> Personally, I prefer to build eclipse plugin using a feature, in this case the
> advantage is that the bundle is automatically jarred, another advantage is to be
> more consistent with our other packages. So is there any technical reason to not
> use a feature instead of these two ant target?

i don't recall- i'll take a look.

> - MUST: A package must own all directories that it creates. If it does not
> create a directory that it uses, then it should require a package which does
> create that directory. Refer to the Guidelines for examples.
> 
> NOK. You should add this line to the files section, take a look on the patch.
> %dir %{eclipse_base}/plugins/de.loskutov.anyedit.AnyEditTools_%{version}

AFAICT, this requirement is already met.  rpm shows that this directory is
already owned by the rpm:
# rpm -qf /usr/share/eclipse/plugins/de.loskutov.anyedit.AnyEditTools_1.8.2
eclipse-anyedit-1.8.2-1.el5

am i missing something?

> - MUST: Permissions on files must be set properly. Executables should be set
> with executable permissions, for example. Every %files section must include a
> %defattr(...) line.
> 
> NOK. The last parameter of the defattr directive set permissions on directories,
> something like %defattr(-,root,root,-) seems better.
> See http://docs.fedoraproject.org/developers-guide/ch-rpm-building.html  for
> more information about defattr.

i believe the package currently meets this requirement, because it already
includes a %defattr line.  however, i will include your suggestion because it
does seem more explicit.  thank you for bringing defattr's fourth argument to my
attention- i didn't know it existed. :)

would you be interested in co-maintaining this package?

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.