Enable XML_ATTR_INFO in build #264
Conversation
There was a problem hiding this comment.
I notice that I'm not 100% sure about these open questions:
- should the new default be ON or OFF for
XML_ATTR_INFO? It's been OFF so far but extending the static.deffiles would need ON for a default (unless we drop the Visual Studio files build system and only generate those through CMake) - how critical is it to get both
XML_ATTR_INFOcovered by CI (Add CMake/Autotools build options for define XML_ATTR_INFO #244)? I'm tending towards need for coverage. The pull request does not yet do that. - should we drop and auto-enable
XML_ATTR_INFOcode for everyone, instead?
I appreciate your input!
expat/coverage.sh
Outdated
| ${unicode_enabled} \ | ||
| && configure_args+=( CPPFLAGS='-DXML_UNICODE -DXML_UNICODE_WCHAR_T' ) | ||
| && configure_args+=( CPPFLAGS+='-DXML_UNICODE -DXML_UNICODE_WCHAR_T' ) |
There was a problem hiding this comment.
That second += looks wrong to me. Does configure support that?
Also gave it a try:
# ./configure CPPFLAGS=X CPPFLAGS+=Y
configure: error: invalid variable name: `CPPFLAGS+'There was a problem hiding this comment.
Updated review comments and uploaded the change
|
Addressed below comments and updated #244 pull request.
Please review the same.
|
|
I thought about this some more and search the internet for In my eyes to go forward we should:
This happens to be in line with #244. I'll review the latest changes on the base of this comment now. |
There was a problem hiding this comment.
Please check my comment at #264 (comment) and the review comments below. Thank you!
|
Addressed review comments in latest patchset.
|
There was a problem hiding this comment.
Looks pretty good to me by now, some formatting detail comments below.
Please not that to close #244, these new build flags will need to be used by the CI, which this pull request does not yet do. We can do it in here or in a new pull request after merging, as you like.
Best!
enable XML_ATTR_INFO in build to cover XML_ATTR_INFO in CI Signed-off-by: Mohammed Khajapasha <mohammed.khajapasha@intel.com>
|
Hi,
I have updated #264 change with below review comments,
As per pull request #264, we have enabled XML_ATTR_INFO flag into auto make & cmake build system.
Can we have a separate issue for integrating XML_ATTR_INFO flag into CI, once above change has been merged ?
Created #273 issue to cover XML_ATTR_INFO into CI.
|
|
Merged in df83b43 with tiny changes on top, thanks! |
enable XML_ATTR_INFO in build to cover XML_ATTR_INFO in CI
Signed-off-by: Mohammed Khajapasha mohammed.khajapasha@intel.com