Skip to content
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

IO: use Boost library for reading/writing configuration files #442

Merged
merged 7 commits into from
Feb 15, 2024

Conversation

andrea-iob
Copy link
Member

Using boost allows to drop libxml2 and RapidJOSN dependencies. These two dependencies are tricky to install on Windows, dropping them will make it easy to install bitpit in Windows.

@roccoarpa
Copy link
Contributor

Hi, apologies, I prefer to leave the review to the others.

@andrea-iob andrea-iob force-pushed the io.configuration.with.boost branch from 600d0f8 to ab5dac0 Compare January 18, 2024 22:15
@andrea-iob andrea-iob force-pushed the io.configuration.with.boost branch from ab5dac0 to 81afc0f Compare January 19, 2024 10:12
@edoardolombardi
Copy link
Member

The pull modifications are fine. Anyway, as it is, it's compiling only for Linux, even if its goal is to simplify the Windows compilation.
Thus, as the pull is related to issue affecting Windows version of the library, it should be rebased on a msvc compliant branch. The current branch rebased on top of https://github.com/optimad/bitpit/tree/general.msvc.compiling.for.AIob can be found in https://github.com/optimad/bitpit/tree/io.configuration.with.boost.msvc, @andrea-iob.

Furthermore, as libxml2 and rapidjson library are no more needed, the download of them through Nuget tool during project configuration, as done in the mscv compliant branches should be removed.

The pull should be merged together (or after, in any case rebased on) the complete support of Windows OS.
Please, consider that the msvc related branch/es need continuous rebases on top of the master branch to be kept aligned and often conflicts happen, mostly in cmake project files.

@andrea-iob
Copy link
Member Author

This pull request is a preparatory step for adding Windows support and should be merged before adding Windows support. The branch the will allow to compile bitpit on Windows will start from here and will introduce the needed changes for adding Windows support. The sole purpose of these changes is to avoid using Nuget in that future branch (libxml2 and RapidJSON were working fine on Linux).

@edoardolombardi
Copy link
Member

In my opinion it's not a good practice coding preparatory developments for Windows without testing them on Windows. Indeed, in order to know if their introduction doesn't affect Windows compliant branch, we had to rebase it and test the library compiled on Windows for our purposes.
The Windows porting should have higher priority than interventions aimed to the optimization of Windows compliancy as this one.
As the branch on Linux and the Windows rebase are ok, I'm approving, but, in order to avoid external rework needed to test the pulls, I think the choice of the prioritization should be reviewed.

@andrea-iob andrea-iob merged commit 0721440 into master Feb 15, 2024
6 checks passed
@andrea-iob andrea-iob deleted the io.configuration.with.boost branch February 15, 2024 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants