Subpackages consolidation #34
Conversation
This allows to not expose "simplebuffer.h"
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly consolidates several sub-packages into the ini_config library, which simplifies the build system and package structure. The changes in Makefile.am and the RPM spec file are mostly on the right track. However, I've identified a few issues that could break the build or introduce inconsistencies. Specifically, configure.ac references m4 macros that have been removed, the RPM spec file retains global definitions for some merged libraries, and there's an inconsistency in how internal headers are handled in Makefile.am. I've provided detailed comments and suggestions to address these points.
alexey-tikhonov
force-pushed
the
merge-ini_config
branch
from
598f998 to
da134b5
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully consolidates the path_utils, collection, refarray, and basicobjects sub-packages into the ini_config library. The changes are extensive, touching the build system, packaging, and symbol visibility, but they appear to be correctly and carefully implemented. The public API has been cleaned up by making many symbols internal, and the refarray API is intentionally kept public as described. The use of Obsoletes in the RPM spec file is a good practice that will ensure a smooth upgrade path for users. The library versioning has been updated correctly to reflect the ABI changes. Overall, this is a solid refactoring that improves the structure of the project. I have not found any high or critical issues in these changes.
alexey-tikhonov
force-pushed
the
merge-ini_config
branch
from
1770d6d to
d1e1a88
Compare
| @@ -1,46 +1,5 @@ | |||
| INI_CONFIG_1.1.0 { | |||
There was a problem hiding this comment.
Hi,
is there a reason to just jump to 1.4.0 and not to 2.0.0?
bye,
Sumit
There was a problem hiding this comment.
If you think it would fit better I don't object.
My grounds were like "no new features introduced".
There was a problem hiding this comment.
If you think it would fit better I don't object. My grounds were like "no new features introduced".
Hi,
I'm not quite sure here, but I think removed interfaces might be more important in this respect. If I understand it correctly if the 1.4.0 version can be used as a drop-in replacement for 1.1.0 i.e. binaries compiles against 1.1.0 will still work, using 1.4.0 is ok. But if a recompilation is needed then 2.0.0 would be better.
bye,
Sumit
|
Hi, the rpm build is successful but has the following warnings: bye, |
This I will fix.
Hm... is it new..? |
I do not see those messages when doing |
alexey-tikhonov
force-pushed
the
merge-ini_config
branch
3 times, most recently
from
0570e15 to
a891398
Compare
Fixed. |
alexey-tikhonov
force-pushed
the
merge-ini_config
branch
from
a891398 to
81daf00
Compare
I "fixed" this but Doxygen docs for 'refarray' API are now lost... |
might be able to bring it back. |
Only leave public what is (or might be potentially) used by SSSD and gssproxy. This bumps both .so major version and 'INI_CONFIG_' symbols version.
alexey-tikhonov
force-pushed
the
merge-ini_config
branch
from
81daf00 to
53f2b1f
Compare
Thank you. This seems to work. Fixed and also changed a note about 'ini_config.h': deprecated -> removed. |
sumit-bose
left a comment
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
thank you for the cleanups and the general consolidation, I have no further comments, ACK.
bye,
Sumit
and don't expose its API
and don't expose its API
and don't expose its API
API is still exposted.
to dist_noinst_HEADERS
in the description of 'ini_config' package as well
alexey-tikhonov
force-pushed
the
merge-ini_config
branch
from
53f2b1f to
6b8a2ef
Compare
4 participants
This PR merges
path_utils,collection,refarray, andbasicobjectsinto theini_configlibrary.refarray's API is still exposed (viaini_configlib) intentionally as it's used byini_configpublic API.API of other libs and parts of
ini_configAPI are made private.