Spec URL: http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng.spec SRPM URL: http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng-4.8.1-0.fc21.src.rpm Description: ProxyChains NG is based on ProxyChains. ProxyChains NG hooks network-related (TCP only) libc functions in dynamically linked programs via a preloaded DSO (dynamic shared object) and redirects the connections through one or more SOCKS4a/5 or HTTP proxies. Since Proxy Chains NG relies on the dynamic linker, statically linked binaries are not supported. Fedora Account System Username: pranvk This is my first package and I need a sponsor.
Koji scratch build here : http://koji.fedoraproject.org/koji/taskinfo?taskID=7703510
Here are some intial findings: - Please add the URL of your upstream issue about the fsfe patch to the SPEC together with a date stamp to make it obvious that the patch is on its way to upstream - Since github seems to be the main page for support etc. please use it as URL: https://github.com/rofl0r/proxychains-ng - The release should start with "1" - You mix the usage of variables with macros, e.g. both $RPM_BUILD_ROOT and %{buildroot} is used, please use either macros or variables - Do you know the %make_install macro? https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used - the %optflags are not completely honoured: cc -shared -fPIC -Wl,--no-as-needed -ldl -lpthread -Wl,-soname=libproxychains4.so -o libproxychains4.so src/nameinfo.o src/version.o src/core.o src/common.o src/libproxychains.o src/shm.o src/allocator_thread.o src/ip_type.o src/stringdump.o src/hostentdb.o src/hash.o src/debug.o cc src/main.o src/common.o -o proxychains4 Reference: https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags - The package is probably also a candidate for PIE (%global _hardened_build 1) - Please perform informal reviews and link to them here as described in the wiki: https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Get_Sponsored Don't be afraid because of the comments, there are a lot of details that need to be kept in mind which takes a little time to know them all. :-)
(In reply to Till Maas from comment #2) > Here are some intial findings: > > - Please add the URL of your upstream issue about the fsfe patch to the SPEC > together with a date stamp to make it obvious that the patch is on its way > to upstream > - Since github seems to be the main page for support etc. please use it as > URL: > https://github.com/rofl0r/proxychains-ng > - The release should start with "1" > - You mix the usage of variables with macros, e.g. both $RPM_BUILD_ROOT and > %{buildroot} is used, please use either macros or variables > - Do you know the %make_install macro? > https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_. > 25makeinstall_macro_should_not_be_used > - the %optflags are not completely honoured: > cc -shared -fPIC -Wl,--no-as-needed -ldl -lpthread > -Wl,-soname=libproxychains4.so -o libproxychains4.so src/nameinfo.o > src/version.o src/core.o src/common.o src/libproxychains.o src/shm.o > src/allocator_thread.o src/ip_type.o src/stringdump.o src/hostentdb.o > src/hash.o src/debug.o > cc src/main.o src/common.o -o proxychains4 > > Reference: https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags > - The package is probably also a candidate for PIE (%global _hardened_build > 1) > > - Please perform informal reviews and link to them here as described in the > wiki: > https://fedoraproject.org/wiki/ > Join_the_package_collection_maintainers#Get_Sponsored > All issues taken care in new set of SPEC, SRPM. Spec URL: http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng.spec SRPM URL: http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng-4.8.1-1.fc21.src.rpm Except, I am little doubtful still about if I have taken care of optflags correctly or not. Here is the link to informal review for a package I did some days ago : https://bugzilla.redhat.com/show_bug.cgi?id=909887#c4
New remarks: - The RPM changelog does not match the release now - You should add a list of changes to the the %changelog - You can check whether the %optflags are honoured in the build logs of a scratch build for example, also I looked at the makefile, the flags are missing in line 87 afaics: https://github.com/pranavk/proxychains-ng/blob/master/Makefile#L87 - so the file probably needs to be patched. The configure script seems to properly get the optflags.
(In reply to Till Maas from comment #4) > New remarks: > > - The RPM changelog does not match the release now > - You should add a list of changes to the the %changelog > - You can check whether the %optflags are honoured in the build logs of a > scratch build for example, also I looked at the makefile, the flags are > missing in line 87 afaics: > https://github.com/pranavk/proxychains-ng/blob/master/Makefile#L87 - so the > file probably needs to be patched. The configure script seems to properly > get the optflags. Thanks for the review. I updated the SPEC, SRPM again accordingly. Spec URL: http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng.spec SRPM URL: http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng-4.8.1-1.fc21.src.rpm
- The makefile patch needs to be submitted upstream and a note needs to be added to the patch - The changelog entries need contain new lines only with dashes: http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs - the CFLAGS... on the line with 'make %{?_smp_mflags} CFLAGS="$RPM_OPT_FLAGS" ' is not needed with the makefile patch
(In reply to Till Maas from comment #6) > - The makefile patch needs to be submitted upstream and a note needs to be > added to the patch > - The changelog entries need contain new lines only with dashes: > http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs > - the CFLAGS... on the line with 'make %{?_smp_mflags} > CFLAGS="$RPM_OPT_FLAGS" ' is not needed with the makefile patch Fixed again. Spec URL: http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng.spec SRPM URL: http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng-4.8.1-1.fc21.src.rpm
Please increase the release the next time when you adjust the SPEC, it makes it easier to compare SRPMs. If you find the time, please perform some more informal reviews (preferable also from someone else who is not yet sponsored, I need to catch-up with all current review guidelines and if nobody beats me to it I will sponsor you if the informal reviews look good.
Another informal review : https://bugzilla.redhat.com/show_bug.cgi?id=1144000#c1
COPYING probably needs to be included with %license, see: https://fedoraproject.org/wiki/Packaging:LicensingGuidelines Do you plan to do more informal reviews?
I was wondering if all these informal reviews needs to be done manually only or can I use fedora-review tool for this ?
Informal review : https://bugzilla.redhat.com/show_bug.cgi?id=1186900
Informal review : https://bugzilla.redhat.com/show_bug.cgi?id=1186819
I also updated the SPEC, SRPM. Spec URL: http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng.spec SRPM URL: http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng-4.8.1-2.fc21.src.rpm
(In reply to Till Maas from comment #10) > COPYING probably needs to be included with %license, see: > https://fedoraproject.org/wiki/Packaging:LicensingGuidelines Though fedora-review tool I am using insist that I place COPYING file under %doc, I have currently moved COPYING to %license as of now to match with current LicensingGuidelines. Let me know if something else needs to be done in this case
Another informal review : https://bugzilla.redhat.com/show_bug.cgi?id=1185662
Another informal review : https://bugzilla.redhat.com/show_bug.cgi?id=1181927
(In reply to Pranav Kant from comment #15) > (In reply to Till Maas from comment #10) > > COPYING probably needs to be included with %license, see: > > https://fedoraproject.org/wiki/Packaging:LicensingGuidelines > > Though fedora-review tool I am using insist that I place COPYING file under > %doc, This is a known bug: https://fedorahosted.org/FedoraReview/ticket/244 (In reply to Pranav Kant from comment #11) > I was wondering if all these informal reviews needs to be done manually only > or can I use fedora-review tool for this ? You can use the tool but you need to be aware that it might not be 100% correct. Thank you for the many informal reviews, I will take a look at them.
I took a look at your informal reviews. When you use fedora-review, you need to manually complete the review template, i.e. if there is a empty box like "[ ]", you need to manually evaluate whether this is not applicable, correct or not correct. Therefore can you please complete your informal reviews, so that the checklists are complete? If you have any questions, feel free to ask.
Also please explain in your reviews, whether you consider the rpmlint output to be ok or if not, what needs to be done. Also for e.g. the Provides/Requires output, please comment whether this means there is a problem or not.
Sorry, I was a bit loose this time doing reviews. Will let you know of other informal reviews I make as soon as I get time.
More informal reviews: https://bugzilla.redhat.com/show_bug.cgi?id=1187082 https://bugzilla.redhat.com/show_bug.cgi?id=1187084 Please review my reviews. :)
Another informal review : https://bugzilla.redhat.com/show_bug.cgi?id=1185550
Another informal review : https://bugzilla.redhat.com/show_bug.cgi?id=1176593
Another informal review : https://bugzilla.redhat.com/show_bug.cgi?id=1193986
Using obsoletes in a spec is not necessary, UNLESS you're renaming something. Please drop it. Yo can get rid of a false positive rpmlint warning: proxychains-ng.src:63: W: macro-in-%changelog %license by prepending another % before %license.
(In reply to Matthias Runge from comment #26) > Using obsoletes in a spec is not necessary, UNLESS you're renaming > something. Please drop it. > > Yo can get rid of a false positive rpmlint warning: proxychains-ng.src:63: > W: macro-in-%changelog %license > by prepending another % before %license. Thanks for the review. I have fixed this up, and updated the links.
Pranav, could you please increase the release and add your changes to %changelog? That makes reviewing easier. Is proxychanins-ng going to replace proxychains? or is it installable in parallel? I must have missed that in my earlier comment. With your provides: proxychains line, it will even become installed, when someone does yum install proxychains That is mostly undesirable (and you should remove that, too).
(In reply to Matthias Runge from comment #28) > Is proxychanins-ng going to replace proxychains? or is it installable in > parallel? I must have missed that in my earlier comment. sorry for the confusion. proxychains-ng is meant to replace proxychains i.e. the current plan is to retire proxychains when this review is finished.
(In reply to Matthias Runge from comment #28) > Pranav, could you please increase the release and add your changes to > %changelog? > That makes reviewing easier. I already increased the release and added my changes to %changelog, but forgot to post the new links. Sorry for that. Here are the new links : http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng.spec http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng-4.8.1-3.fc21.src.rpm
The optflags patch was not accepted upstream or commented in the spec. You seem to missed the upstream comment at: https://github.com/rofl0r/proxychains-ng/pull/37#issuecomment-58201846 Here is a patch that seems to be more correct: https://github.com/rofl0r/proxychains-ng/pull/51 - the downstream versioning patch was also not accepted by upstream and the reasons sounds valid to me. However it is probably a good idea to ask on the Fedora devel list for opinions of others - the library needs to be installed with executable flags set in Fedora so RPM strips them, otherwise we have this rpmlint warning: proxychains-ng.x86_64: W: unstripped-binary-or-object /usr/lib64/libproxychains4.so.0
Thanks for the comments. Here are the updated links with all your suggestions addressed : http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng.spec http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng-4.8.1-4.fc21.src.rpm (In reply to Till Maas from comment #31) > - the downstream versioning patch was also not accepted by upstream and the > reasons sounds valid to me. However it is probably a good idea to ask on the > Fedora devel list for opinions of others I will fix this accordingly as soon as I get any suggestion, but till then just sticking with the patch.
Umm?? It seems that libproxychains4.so is just LD_PRELOAD'ed. In this case adding soversion is not needed. https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Downstream_.so_name_versioning Quote: In cases where upstream ships unversioned .so *library* (so this is not needed for plugins, drivers, etc.) Just * move libproxychains4.so to some application-private directory (like %_libdir/%name or so) * Adjust src/proxyresolv script and perhaps dll_dirs[] in src/main.c
Putting non-development .so(.X) under %_libdir is regarded as it is a _library_ and in that case unversioned _library_ is strongly discouraged. However in most cases .so files to be dlopen()ed is not expected to be used as a _library_. In such cases, just moving it to application-specific directory (so that such .so file is not to be used as a _library_) is enough.
s/dlopen()ed/dlopen()ed or preloaded/
Well, anyway the current version_so.patch is wrong and breaks /usr/bin/proxyresolv4 functionality because it says: ------------------------------------------------- 17 export LD_PRELOAD=libproxychains4.so 18 dig $1 @$DNS_SERVER +tcp | awk '/A.?[0-9]+\.[0-9]+\.[0-9]/{print $5;}' ------------------------------------------------- but the module name is changed to libproxychains4.so.X, so libproxychains4.so cannot be preloaded.
> https://github.com/rofl0r/proxychains-ng/issues/46 That request is difficult to understand. It doesn't explain *why* a version is requested, and it doesn't explain what "such problems" refers to. An unversioned shared lib is not a problem. Unless it changes ABI often, in which case automatic dependencies on the unversioned lib soname would be very weak. It is also wrong to claim "unversioned so file which usually goes in -dev, -devel packages". There is no such thing as "usually" for .so files. It really depends on *how* (and when) such .so files (or symlinks) are used: https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages Versioning shared libs does not solve any problem related to -devel packages. For example, imagine you had multiple versions of a library. You still could not have two non-versioned .so symlinks in the -devel packages, because they would conflict with eachother by pointing at different link targets. Another solution for that would be needed (such as hiding files from default search paths). > move libproxychains4.so to some application-private directory > (like %_libdir/%name or so) That would move it outside runtime linker's search path. Stuff like LD_PRELOAD=libproxychains4.so would fail and would need to be patched to add the full path. Not good if it were not done by upstream. [...] IMHO, there is nothing wrong with keeping libproxychains4.so in %_libdir.
So what I am saying is that libproxychains4.so should be go away from %_libdir and go to application specific directory not to unneededly pollute system-wide %_libdir and patch accordingly, unless this .so is meant to be used by other application.
Thanks for your valuable comments. I have removed the .so versioning now since the fedora package guidelines recommends versioning only on usable libraries, and this is not a library. "In cases where upstream ships unversioned .so *library* (so this is not needed for plugins, drivers, etc.)" And following Michael's comment : "there is nothing wrong with keeping libproxychains.so in $_libdir" I am keeping it in %_libdir for the time being, though I will try to convince the upstream to LD_PRELOAD it, by default, from application-specific directory. Here are the updated links: http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng.spec http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng-4.8.1-5.fc21.src.rpm
> I will try to convince the upstream to LD_PRELOAD it, by default, > from application-specific directory. Well, that is possible already with a changed %_libdir (see diff below) at configuration time, but you include the legacy script proxyresolv that is not installed by default. And that one relies on runtime linker's search path list. --- proxychains-ng.spec.orig 2015-03-16 14:11:09.000000000 +0100 +++ proxychains-ng.spec 2015-03-16 15:59:22.199956553 +0100 @@ -37,24 +37,20 @@ %patch1 -p1 %build -%configure --disable-static +%configure --disable-static --libdir=%{_libdir}/%{name} make %{?_smp_mflags} %install %make_install install-config -%{__install} -Dm 0755 src/proxyresolv %{buildroot}%{_bindir}/proxyresolv4 -%{__install} -Dm 0755 libproxychains4.so %{buildroot}%{_libdir}/libproxychains4.so - -%post -p /sbin/ldconfig -%postun -p /sbin/ldconfig +chmod +x %{buildroot}%{_libdir}/%{name}/libproxychains4.so %files %license COPYING %doc AUTHORS README TODO %config(noreplace) %{_sysconfdir}/proxychains.conf %{_bindir}/proxychains4 -%{_bindir}/proxyresolv4 -%{_libdir}/libproxychains4.so +%dir %{_libdir}/%{name} +%{_libdir}/%{name}/libproxychains4.so %changelog * Mon Mar 16 2015 Pranav Kant <pranav913> 4.8.1-5
So, should I make appropriate changes, as mentioned above, to spec file to move it in a application specific directory ? Or leave it as it is ?
(In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #40) > > I will try to convince the upstream to LD_PRELOAD it, by default, > > from application-specific directory. > > Well, that is possible already with a changed %_libdir (see diff below) at > configuration time, but you include the legacy script proxyresolv that is > not installed by default. And that one relies on runtime linker's search > path list. I made appropriate changes as suggested and here are the updated links : http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng.spec http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng-4.8.1-6.fc21.src.rpm Changes in this revision : * Tue Mar 17 2015 Pranav Kant <pranav913> 4.8.1-6 - Remove legacy script - proxyresolv4 - Move .so file to application-specific directory
Any update on this ?
Here are the corresponding successfull koji builds : http://koji.fedoraproject.org/koji/taskinfo?taskID=9377285 http://koji.fedoraproject.org/koji/taskinfo?taskID=9377308 http://koji.fedoraproject.org/koji/taskinfo?taskID=9377321 http://koji.fedoraproject.org/koji/taskinfo?taskID=9377262
I unassign myself due to lack of time, but I will continue once I find more.
Pranav, are you still waiting for someone to review this?
(In reply to Mamoru TASAKA from comment #46) > Pranav, are you still waiting for someone to review this? Yeah. The package, though, has already passed through a lot of reviews, but it still needs the approval of someone who can sponsor me.
* Modifying license file downstream - Well, even if rpmlint complains somehow, I am strongly against modifying license text downstream arbitrarily. License text change must be done upstream. Please pull the _actual_ upstream fix with the URL: https://github.com/rofl0r/proxychains-ng/commit/567935b1abb93af561600081461a46b89468b9ca * Relation with "proxychains" (In reply to Till Maas from comment #29) > (In reply to Matthias Runge from comment #28) > > > Is proxychanins-ng going to replace proxychains? or is it installable in > > parallel? I must have missed that in my earlier comment. > > sorry for the confusion. proxychains-ng is meant to replace proxychains i.e. > the current plan is to retire proxychains when this review is finished. - Then please add some "Obsoletes" entry against proxychains. * NOTE: - Currently %_sysconfdir/proxychains.conf is also owned by the "old" proxychains, so installing both proxychains and proxychains-ng sees a conflict on %_sysconfdir/proxychains.conf . Anyway, adding "Obsoletes" or "Conflicts" against proxychains is currently needed (unless config file on proxychains-ng is changed). * Distro-specific configuration > Adjust ~/.proxychains/proxychains.conf for your Proxy and use ProxyChains NG > with > proxychains4 application - If this application cannot be used as it is and some configuration by user is needed, I think some explanation file like "README.fedora" or so should be created, such explanation is not suitable for writing in Summary or %description.
(In reply to Mamoru TASAKA from comment #48) > * Distro-specific configuration > > Adjust ~/.proxychains/proxychains.conf for your Proxy and use ProxyChains NG > > with > > proxychains4 application > - If this application cannot be used as it is and some configuration > by user is needed, I think some explanation file like "README.fedora" > or so should be created, such explanation is not suitable for writing > in Summary or %description. This configuration explanation is also available in README file that is shipped with the package. Do we still need to add a separate README.fedora specially for this configuration information again ?
What I am saying is that information for configuration is not suitable for Summary or %description. * If some _Fedora-specific_ configuration by each user or system admin is needed, add "README.fedora". * If some configuration by each user or system admin is needed by it is already written on the files shipped by the upstream, just say "read this file for configuration" on %description, or may be such notich is just not needed. * By the way, unless there is some reason, please make the installed package work "as it is", without manual configuration by user.
Updated links : http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng.spec http://glug.nith.ac.in/~pranavk/fedora/proxychains-ng-4.8.1-7.fc22.src.rpm
I have some unclarified issues with the code so I mailed to the authority. Please wait for a few days...
(In reply to Mamoru TASAKA from comment #52) > I have some unclarified issues with the code so I mailed to the authority. > Please wait for a few days... Now the "unclarified issues" I wrote here is assigned the ID CVE-2015-3887 . Please fix this. ref: http://seclists.org/oss-sec/2015/q2/415 http://seclists.org/oss-sec/2015/q2/430
Fixed CVE-2015-3887 Updated links : https://pranvk.fedorapeople.org/packager/proxychains-ng.spec https://pranvk.fedorapeople.org/packager/proxychains-ng-4.8.1-8.fc22.src.rpm
Approving. ----------------------------------------------------- This package (proxychains-ng) is APPROVED by mtasaka ----------------------------------------------------- Please follow https://fedoraproject.org/wiki/Join_the_package_collection_maintainers from "Add Package to Source Code Management (SCM) system and Set Owner". As this is a first package you are to maintain, please add me as InitiallCC member.
And please talk with the current proxychains maintainer so that proxychains is to be retired on F-23 once this package is imported into Fedora repository.
New Package SCM Request ======================= Package Name: proxychains-ng Short Description: Redirect connections through proxy servers Upstream URL: https://github.com/rofl0r/proxychains-ng Owners: pranvk Branches: f21 f22 InitialCC: mtasaka
Git done (by process-git-requests).
proxychains-ng-4.8.1-8.fc22 has been submitted as an update for Fedora 22. https://admin.fedoraproject.org/updates/proxychains-ng-4.8.1-8.fc22
proxychains-ng-4.8.1-8.fc21 has been submitted as an update for Fedora 21. https://admin.fedoraproject.org/updates/proxychains-ng-4.8.1-8.fc21
proxychains-ng-4.8.1-8.fc21 has been pushed to the Fedora 21 testing repository.
proxychains-ng-4.8.1-8.fc22 has been pushed to the Fedora 22 stable repository.
proxychains-ng-4.8.1-8.fc21 has been pushed to the Fedora 21 stable repository.