Created attachment 162267 [details] Diff patch between current port version and upgrade to v2.3.2_1 Port revision update to include patches in aid of clearing clang compiler warnings. No need to test using poudriere since it's only minor code changes which only improve the compile process by clearing warnings. Has been runtime tested in procuction for several days with no issues so far.
Thanks for your submission Euan. * -Werror should not be in CFLAGS * Could you place these in a single CFLAGS variable assignment, optionally on seperate lines terminated by \ *if* you think its absolutely necessary Also, do you plan on submitting these warnings upstream so they can be fixed in code or addressed in another way? If so, please add an issue or pull-request URL reference just above the CFLAGS assignment so they can be removed once upstreamed
Yes, he's working with theraven to get them upstream. David prepared prelim version of those patches when he was preparing a port for sope and sogo and submitter try to get them upstream. btw, testing@work
(In reply to Kubilay Kocak from comment #1) The CFLAGS are purely for my testing and development purposes. They should be commented out in the official port. I have them on seperate lines so that I can easily toggle them individually as I include/exclude different types of warnings. Since the build stops as soon as it hits the first warning. I can modify as you suggest if I have to. The only reason I left them in there is so that I don't lose track of them. Adding a comment about the upstream process, as you suggest, is a good idea. Yes, I am in the process of getting these upstream. I have already contacted the lead developer and he is very accommodating so far. Since there are a lot of patches and some platform / build environment issues which potentially conflict with the support of old RHEL versions, it may take some time. I'm doing it in stages. David Chisnall has been helping me with this process so far. There is not one single pull request, there is only one so far, since I'm doing it in stages. There will be more to come in the couple of weeks I imagine. But it may happen faster, I can't tell. In the meantime, I'd like to get the port for version 2.3.2 updated. The upstream changes will filter down into new versions over time. It may be a while before the next release. And the next release may be the first of the 3.x series, which I think I heard, has a new UI. This could pose some problems in itself. Perhaps we'll have to create a legacy port for 2.x. I for one will not be moving my user base to a new UI so soon. I will stay on 2.3.x for some time yet.
@Euan, thanks for your response. Proposed changes to ports are always viewed in the context of: will be pushed to users, and should be 'production' ready. In that context, the content, format and structure of changes should strictly be for consumers, not maintainers/committers. Some committers do however, sometimes include certain bits of a ports contents in an .if (MAINTAINER_MODE) conditional, but are usually only included if related to testing / QA, updating a port, or functionality that might otherwise be valuable to users as well, on an ongoing basis. In this case, CFLAGS are trivially overridable by the user in /etc/make.conf, and the warnings themselves (while padding build logs), are not explicitly required to be fixed in the port, as opposed to upstream directly. Having said that, its a good thing that you'd like to refine the port and it's compilation to that extent, but in doing so does not preclude removing -Werror in particular, as it is not safe across architectures, versions and environments, and can (and has) caused issues with package building before. TLDR; * Changes should be 'production ready for users' by default. * If it's not strictly necessary, causing impact, or providing additional value, consider upstreaming first by default. * No -Werror in CFLAGS (some ports explicitly remove it from inappropriate upstream defaults)
About this: If it's not strictly necessary, causing impact, or providing additional value, consider upstreaming first by default. David provided his patches to Euan because he assumes that running the application without them might have security implications. With a complex application like this it is a useful workflow to have it in the tree, then patch it up to some level, then upstream, so that operational experience can be gained in parallel to bringing it to some 'perfecter' shape. Nothing will ever be 'perfect', anyway 8-)
@Kurt The security implications were not mentioned until your comment unless I missed something. It's good those details are noted now, which helps the issue progress I want to note that the TLDR items mentioned at the end of my last comment are all 'all else being equal, rules of thumb' comments. Potential security impacts of course might be grounds to include certain changese if they are warranted and I would trust a FreeBSD committer on the claim without requiring justification. The goal is a clear rationale to accept the proposed changes. In that regard alone: * CFLAGS modifications are fine, given a rationale, and considering/testing for potential impacts * -Werror needs to be removed Pending updated patch from maintainer
Adding appropriate keywords and flags in case this change needs to be MFH'd
(In reply to Kubilay Kocak from comment #6) The _potential_ security implications are just that (AFAIK). There is no proven exploitable security flaw yet. Only that the kinds of coding errors the compiler warnings are highlighting are systematic of causing security issues. Along with less than obvious bugs. So, it's just good practice to clear these up and make the upstream code better quality. These compiler warnings aren't even the tip of the iceberg of a proper security audit. But every little helps. Marking the port with a security warning would be a premature assumption. There are no outstanding CVE's AFAIK. As for the CFLAGs, I will remove them from the Makefile and add them to my internal notes I keep on my github ports development branch. I'll update the patches for devel/sope www/sogo just now.
@Euan, agreed. There was also no proposal to add a security warning to the port. I just want to make it clear that I am commenting, as other issue tracker triagers/committers do, *purely* and *only* on the basis of getting an issue resolved and a proposed changeset ready for that if required, accurately and correctly. In that context I am not specifically concerned with the contents of any changeset perse, just that it's appropriate *given* the detail provided in the issue itself. Given "As for the CFLAGs, I will remove them from the Makefile", what changes does that leave to commit, or will only a subset of those no-warning flags be left in the updated patch?
(In reply to Kubilay Kocak from comment #9) It means I will remove all the "CFLAGS+=..." from the Makefile. They were already commented out anyway. So effectively no change to the port will take place, just the neatness to the port users as you suggested. The changes to the port are the ~150 extra patches in "files". These will filter upstream gradually, hopefully in time for the next port version. I think you may be a bit confused. I was not clearing warnings with CFLAGs. The CFLAGs were there to help me identify the compiler warnings during my port development. Then I commented them out before submitting here. The real _clearing_ of the warnings are small code corrections, started by David and continued by myself. They are mostly rudimentary fixes, "C" data type and casting incosistencies. Plenty string formatting fixes. And some other minor code cleanliness, (potential bug fixes).
@Euan I did indeed forgot those parts :)
@Kurt do you want to grab this (Assignee) ?
(In reply to Kubilay Kocak from comment #11) OK, so do you still want the "#commented out" CFLAGs to be removed? I haven't submitted a new diff yet.
(In reply to Kubilay Kocak from comment #12) As soon as I finished testing, as usual. Me testing and probably falling in a black hole due to some unforeseen WTF in my part of the galaxy should prevent nobody from also working on it. Especially given the things coming up until this weekend and next week...
Yes please Euan (I use the needs-patch keyword to denote this) Just trying to line up a committer for you quickly (either Kurt or myself)
Created attachment 162271 [details] Diff patch between current port version and upgrade to v2.3.2_1 Removed "CFLAGS+=" from Makefile.
Ideally I'd like this port update to be committed at the same time as www/sogo (203910). But it's not essential.
Please note I did not intentionally make changes to the blocks depends (above). I think maybe I answered a collision wrongly when I last commented.
Test-building @work
Created attachment 162434 [details] Diff patch between current port version and upgrade to v2.3.2_1 Fixed files/patch-* file names.
A commit references this bug: Author: pi Date: Sun Oct 25 06:47:32 UTC 2015 New revision: 400145 URL: https://svnweb.freebsd.org/changeset/ports/400145 Log: devel/sope: Fix lots of compiler warnings PR: 203909 Submitted by: Euan Thoms <euan@potensol.com> (maintainer) Changes: head/devel/sope/Makefile head/devel/sope/files/patch-sope-appserver_NGObjWeb_DynamicElements_WOHyperlinkInfo.m head/devel/sope/files/patch-sope-appserver_NGObjWeb_WOHttpAdaptor_WOHttpTransaction.m head/devel/sope/files/patch-sope-appserver_NGObjWeb_WebDAV_SoObjectDataSource.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__Associations__WOScriptAssociation.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__Associations__WOValueAssociation.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__DynamicElements__WOGenericElement.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__DynamicElements__WOHyperlink.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__NGHttp__NGHttpHeaderFieldParser.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__NGHttp__NGHttpResponse.h head/devel/sope/files/patch-sope-appserver__NGObjWeb__NGHttp__NGUrlFormCoder.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__SoObjects__SoClassSecurityInfo.h head/devel/sope/files/patch-sope-appserver__NGObjWeb__SoObjects__SoClassSecurityInfo.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__SoObjects__SoComponent.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__SoObjects__SoDefaultRenderer.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__Templates__WOComponentScriptPart.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__Templates__WODParser.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__Templates__WOHTMLParser.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__Templates__WOTemplate.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__Templates__WOxElemBuilder.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WOComponent+Sync.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WOComponent.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WOHTTPConnection.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WOHttpAdaptor__WOHttpAdaptor.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WOHttpAdaptor__WORequest+Adaptor.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WOMailDelivery.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WOResponse.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WOSessionStore.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WOSimpleHTTPParser.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WOStatisticsStore.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WebDAV__SoDAVSQLParser.h head/devel/sope/files/patch-sope-appserver__NGObjWeb__WebDAV__SoDAVSQLParser.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WebDAV__SoWebDAVRenderer.m head/devel/sope/files/patch-sope-appserver__NGObjWeb__WebDAV__SoWebDAVValue.m head/devel/sope/files/patch-sope-appserver__NGObjWeb___WOStringTable.m head/devel/sope/files/patch-sope-appserver__WEExtensions__JSStringTable.m head/devel/sope/files/patch-sope-appserver__WEExtensions__WEMonthOverview.m head/devel/sope/files/patch-sope-appserver__WEExtensions__WEResourceKey.m head/devel/sope/files/patch-sope-appserver__WEExtensions__WEResourceManager.m head/devel/sope/files/patch-sope-appserver__WEExtensions__WETableCalcMatrix.h head/devel/sope/files/patch-sope-appserver__WEExtensions__WETableCalcMatrix.m head/devel/sope/files/patch-sope-appserver__WEExtensions__WETableMatrix.m head/devel/sope/files/patch-sope-appserver__WEExtensions__WETableView__WETableView.m head/devel/sope/files/patch-sope-appserver__WOExtensions__WOTabPanel.m head/devel/sope/files/patch-sope-core_EOControl_EOSortOrdering.m head/devel/sope/files/patch-sope-core_EOControl_EOValidation.m head/devel/sope/files/patch-sope-core_NGExtensions_FdExt.subproj_NSString+Encoding.m head/devel/sope/files/patch-sope-core_NGExtensions_NGCalendarDateRange.m head/devel/sope/files/patch-sope-core_NGStreams_NGActiveSocket.m head/devel/sope/files/patch-sope-core__EOControl__EOFetchSpecification.m head/devel/sope/files/patch-sope-core__EOControl__EOGlobalID.m head/devel/sope/files/patch-sope-core__EOControl__EOKeyComparisonQualifier.m head/devel/sope/files/patch-sope-core__EOControl__EOKeyGlobalID.m head/devel/sope/files/patch-sope-core__EOControl__EOKeyValueQualifier.m head/devel/sope/files/patch-sope-core__EOControl__EONull.m head/devel/sope/files/patch-sope-core__EOControl__EOObserver.m head/devel/sope/files/patch-sope-core__EOControl__EOOrQualifier.m head/devel/sope/files/patch-sope-core__EOControl__EOSQLParser.h head/devel/sope/files/patch-sope-core__EOControl__EOSQLParser.m head/devel/sope/files/patch-sope-core__NGExtensions__FdExt.subproj__NGPropertyListParser.m head/devel/sope/files/patch-sope-core__NGExtensions__FdExt.subproj__NSException+misc.m head/devel/sope/files/patch-sope-core__NGExtensions__FdExt.subproj__NSObject+Logs.m head/devel/sope/files/patch-sope-core__NGExtensions__FdExt.subproj__NSObject+Values.m head/devel/sope/files/patch-sope-core__NGExtensions__FdExt.subproj__NSSet+enumerator.m head/devel/sope/files/patch-sope-core__NGExtensions__FdExt.subproj__NSString+misc.m head/devel/sope/files/patch-sope-core__NGExtensions__NGDirectoryEnumerator.m head/devel/sope/files/patch-sope-core__NGStreams__NGBufferedStream.m head/devel/sope/files/patch-sope-core__NGStreams__NGByteBuffer.m head/devel/sope/files/patch-sope-core__NGStreams__NGByteCountStream.m head/devel/sope/files/patch-sope-core__NGStreams__NGCTextStream.m head/devel/sope/files/patch-sope-core__NGStreams__NGConcreteStreamFileHandle.m head/devel/sope/files/patch-sope-core__NGStreams__NGLocalSocketAddress.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EOAdaptor.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EOAttribute.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EODatabase.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EODatabaseChannel.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EODatabaseContext.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EODatabaseFault.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EODatabaseFaultResolver.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EOEntity.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EOFExceptions.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EOFault.h head/devel/sope/files/patch-sope-gdl1__GDLAccess__EOFault.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EOModel.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EOObjectUniquer.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EOPrimaryKeyDictionary.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EORecordDictionary.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EORelationship.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EOSQLExpression.m head/devel/sope/files/patch-sope-gdl1__GDLAccess__EOSQLQualifier.m head/devel/sope/files/patch-sope-gdl1__PostgreSQL__NSData+PGVal.m head/devel/sope/files/patch-sope-gdl1__PostgreSQL__NSString+PostgreSQL72.m head/devel/sope/files/patch-sope-gdl1__PostgreSQL__PostgreSQL72Channel.m head/devel/sope/files/patch-sope-gdl1__PostgreSQL__PostgreSQL72DataTypeMappingException.m head/devel/sope/files/patch-sope-ldap_NGLdap_NGLdapConnection.m head/devel/sope/files/patch-sope-ldap_NGLdap_NGLdapFileManager.m head/devel/sope/files/patch-sope-ldap_NGLdap_NGLdapGlobalID.m head/devel/sope/files/patch-sope-mime_NGImap4_NGImap4Client.h head/devel/sope/files/patch-sope-mime_NGImap4_NGImap4Client.m head/devel/sope/files/patch-sope-mime_NGImap4_NGImap4Functions.m head/devel/sope/files/patch-sope-mime_NGImap4_NGImap4ResponseParser.m head/devel/sope/files/patch-sope-mime__NGImap4__NGImap4Connection.m head/devel/sope/files/patch-sope-mime__NGImap4__NGImap4Context.m head/devel/sope/files/patch-sope-mime__NGImap4__NGImap4FileManager.m head/devel/sope/files/patch-sope-mime__NGImap4__NGImap4Folder.m head/devel/sope/files/patch-sope-mime__NGImap4__NGImap4FolderGlobalID.m head/devel/sope/files/patch-sope-mime__NGImap4__NGImap4Message.m head/devel/sope/files/patch-sope-mime__NGImap4__NGImap4ServerGlobalID.m head/devel/sope/files/patch-sope-mime__NGImap4__NGSieveClient.m head/devel/sope/files/patch-sope-mime__NGMail__NGMailAddress.m head/devel/sope/files/patch-sope-mime__NGMail__NGMailAddressParser.m head/devel/sope/files/patch-sope-mime__NGMail__NGMimeMessage.m head/devel/sope/files/patch-sope-mime__NGMime__NGMimeBodyPart.m head/devel/sope/files/patch-sope-mime__NGMime__NGMimeJoinedData.m head/devel/sope/files/patch-sope-mime__NGMime__NGMimePartGenerator.h head/devel/sope/files/patch-sope-mime__NGMime__NGMimePartGenerator.m head/devel/sope/files/patch-sope-mime__NGMime__NGMimePartParser.m head/devel/sope/files/patch-sope-mime__NGMime__NGMimeRFC822DateHeaderFieldParser.m head/devel/sope/files/patch-sope-xml_SaxObjC_SaxObjectDecoder.m head/devel/sope/files/patch-sope-xml__DOM__DOMQueryPathExpression.m head/devel/sope/files/patch-sope-xml__XmlRpc__NSNotification+XmlRpcCoding.m
Test-builds are all fine. Committed, thanks for the huge patch!