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@xxxxxxxxxxxxxxx 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review