Bug 203909 - [MAINTAINER] devel/sope: Add CFLAGS supressing compiler warnings
Summary: [MAINTAINER] devel/sope: Add CFLAGS supressing compiler warnings
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Kurt Jaeger
URL:
Keywords: easy, patch, patch-ready
Depends on:
Blocks: 203910
  Show dependency treegraph
 
Reported: 2015-10-21 03:49 UTC by Euan Thoms
Modified: 2015-10-25 06:50 UTC (History)
5 users (show)

See Also:
koobs: maintainer-feedback+
koobs: merge-quarterly?


Attachments
Diff patch between current port version and upgrade to v2.3.2_1 (131.74 KB, patch)
2015-10-21 03:49 UTC, Euan Thoms
no flags Details | Diff
Diff patch between current port version and upgrade to v2.3.2_1 (131.32 KB, patch)
2015-10-21 07:40 UTC, Euan Thoms
koobs: maintainer-approval+
Details | Diff
Diff patch between current port version and upgrade to v2.3.2_1 (129.63 KB, patch)
2015-10-24 20:51 UTC, Euan Thoms
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Euan Thoms 2015-10-21 03:49:04 UTC
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.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2015-10-21 04:10:03 UTC
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
Comment 2 Kurt Jaeger freebsd_committer 2015-10-21 04:13:10 UTC
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
Comment 3 Euan Thoms 2015-10-21 04:55:39 UTC
(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.
Comment 4 Kubilay Kocak freebsd_committer freebsd_triage 2015-10-21 05:09:29 UTC
@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)
Comment 5 Kurt Jaeger freebsd_committer 2015-10-21 05:40:41 UTC
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-)
Comment 6 Kubilay Kocak freebsd_committer freebsd_triage 2015-10-21 05:46:05 UTC
@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
Comment 7 Kubilay Kocak freebsd_committer freebsd_triage 2015-10-21 05:52:53 UTC
Adding appropriate keywords and flags in case this change needs to be MFH'd
Comment 8 Euan Thoms 2015-10-21 06:22:20 UTC
(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.
Comment 9 Kubilay Kocak freebsd_committer freebsd_triage 2015-10-21 06:40:25 UTC
@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?
Comment 10 Euan Thoms 2015-10-21 06:57:28 UTC
(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).
Comment 11 Kubilay Kocak freebsd_committer freebsd_triage 2015-10-21 07:00:32 UTC
@Euan I did indeed forgot those parts :)
Comment 12 Kubilay Kocak freebsd_committer freebsd_triage 2015-10-21 07:06:20 UTC
@Kurt do you want to grab this (Assignee) ?
Comment 13 Euan Thoms 2015-10-21 07:07:52 UTC
(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.
Comment 14 Kurt Jaeger freebsd_committer 2015-10-21 07:19:59 UTC
(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...
Comment 15 Kubilay Kocak freebsd_committer freebsd_triage 2015-10-21 07:24:23 UTC
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)
Comment 16 Euan Thoms 2015-10-21 07:40:52 UTC
Created attachment 162271 [details]
Diff patch between current port version and upgrade to v2.3.2_1

Removed "CFLAGS+=" from Makefile.
Comment 17 Euan Thoms 2015-10-21 07:44:03 UTC
Ideally I'd like this port update to be committed at the same time as www/sogo (203910). But it's not essential.
Comment 18 Euan Thoms 2015-10-21 08:56:19 UTC
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.
Comment 19 Kurt Jaeger freebsd_committer 2015-10-24 19:34:03 UTC
Test-building @work
Comment 20 Euan Thoms 2015-10-24 20:51:22 UTC
Created attachment 162434 [details]
Diff patch between current port version and upgrade to v2.3.2_1

Fixed files/patch-* file names.
Comment 21 commit-hook freebsd_committer 2015-10-25 06:48:28 UTC
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
Comment 22 Kurt Jaeger freebsd_committer 2015-10-25 06:50:05 UTC
Test-builds are all fine. Committed, thanks for the huge patch!