-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix compilation failure due to -Werror flag #4777
base: master
Are you sure you want to change the base?
Conversation
Attempt to fix error message: imap/http_dav.c: In function ‘parse_xml_body’: imap/http_dav.c:3819:33: error: initialization discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] 3819 | xmlErrorPtr err = xmlCtxtGetLastError(txn->conn->xml); | Signed-off-by: Athaariq Ardhiansyah <[email protected]>
Attempt to fix error message: /mnt/img/athaariq/cyrus-imapd/docsrc/imap/concepts/deployment/storage.rst:973: WARNING: duplicate term description of MTBF, other instance in glossary /mnt/img/athaariq/cyrus-imapd/docsrc/imap/concepts/deployment/storage.rst:977: WARNING: duplicate term description of HBA, other instance in glossary /mnt/img/athaariq/cyrus-imapd/docsrc/imap/concepts/features/server-aggregation.rst:392: WARNING: duplicate term description of frontend, other instance in glossary /mnt/img/athaariq/cyrus-imapd/docsrc/imap/concepts/features/server-aggregation.rst:397: WARNING: duplicate term description of backend, other instance in glossary Signed-off-by: Athaariq Ardhiansyah <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, CUnit and Cassandane shows no problem.
These steps shouldn't be necessary, the top level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I appreciate the enthusiasm for cleaning up the documentation warnings, but most of your proposed changes make things worse. I've addressed each with individual comments.
The documentation warnings are not what's causing your build to fail. -Werror
only applies to the C compiler, not to sphinx-build.
It looks like your build fails because of the const
warning from http_dav.c. I've asked Ken to have a closer look at this one, because if the object returned by the libxml call is const (or is const in newer versions of libxml), the way we use it might be unsafe.
xmlErrorPtr err = xmlCtxtGetLastError(txn->conn->xml); | ||
const xmlError *err = xmlCtxtGetLastError(txn->conn->xml); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ksmurchison can you have a look at this one please, I'm not familiar with the XML library.
In particular, a few lines later we save err->message
in txn->error.desc
. That might not be safe if we don't have lifetime ownership of the err
object, which the constness might imply.
Since we aren't getting this warning/error outselves, perhaps this is an API change in the XML library that we haven't stumbled upon yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding const seems correct as that is what the API specifies. Since we're getting the error from the XML parser context, and there is a xmlCtxtResetLastError() function, I assume its safe to reference the error until we free the parser context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an information, the Arch distribution uses libxml2 v2.12.3. I see the changes at libxml2 GitLab repository which related to this issue. This is what maintainer's say:
This is a slight break of the API, but users really shouldn't modify the global error struct. The goal is to make xmlLastError use static buffers for its strings eventually. This should warn people if they're abusing the struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding const seems correct as that is what the API specifies. Since we're getting the error from the XML parser context, and there is a xmlCtxtResetLastError() function, I assume its safe to reference the error until we free the parser context.
Alright, thanks for the clarification. Is there anything I can do?
Drop commit requests are fulfilled, please take a look
I tried, but it yells "No build target" error. There is no explanation or verbose message about the problem unfortunately, so I have to do that manually.
The commit 6a9d617 went first, but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the changes and ran the CUnit again. @elliefm let me know if you agree with modification on imap/http_dav.c
. Thanks
can this get a review/merge? |
This is an attempt to get this commands working for building the cyrus-imapd:
Without error caused by warning and
-Werror
flag. Also notes that currently I'm working with upstream's latest stable version of toolchains and dependencies. Thus, the fix might not quite relevant on Debian but definitely needed in rolling release distributions like Arch.