Skip to content

Decouple libConfiguration from libCommon#3

Merged
PascalBoeschoten merged 3 commits intoAliceO2Group:masterfrom
ktf:fix-lib-common-dependency
Feb 7, 2018
Merged

Decouple libConfiguration from libCommon#3
PascalBoeschoten merged 3 commits intoAliceO2Group:masterfrom
ktf:fix-lib-common-dependency

Conversation

@ktf
Copy link
Member

@ktf ktf commented Feb 7, 2018

The dependency of libConfiguration to libCommon is not actually required
as the only usage of libCommon is done by helper tools, not
by libConfiguration itself. This takes that into account and
decouples the two libraries. This is required by the fact that
currently DataSampling has a dependency on libConfiguration but not
libCommon and so this simplifies therefore integration.

The dependency of libConfiguration to libCommon is not actually required
as the only usage of libCommon is done by helper tools, not
by libConfiguration itself. This takes that into account and
decouples the two libraries. This is required by the fact that
currently DataSampling has a dependency on libConfiguration but not
libCommon and this simplifies therefore integration.
@ktf
Copy link
Member Author

ktf commented Feb 7, 2018

@knopers8 this is part of your problem.

@ktf
Copy link
Member Author

ktf commented Feb 7, 2018

@PascalBoeschoten @Barthelemy could you have a look at this? Without this solved, one way or another, I cannot continue integrating @knopers8 stuff and this seems to me as the best ratio between amount of changes and cleanliness of the solution.

SYSTEMINCLUDE_DIRECTORIES
${Boost_INCLUDE_DIR}
${CURL_INCLUDE_DIRS}
${Common_INCLUDE_DIRS}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktf This will break src/Backends/MySql/MySqlBackend.cxx which requires Common/GuardFunction.h

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the GuardFunction is not used so this include can be delete

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that's a leftover import. We can remove it. It's just strange that the CI build isn't broken

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use ScopeExit which is roughly the same thing from boost?

That said, where is GuardFunction actually used? I think this is a spurious include. Can you check if my new commit works (I do not have mysql installed on my laptop)?

This is also another great example of why the backend should be split from the actual frontend API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and of course we should disable / drop the mysql backend now that we decided to go for consul.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktf indeed, the code for mysql can be disabled but I would do it in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Barthelemy agree. I will wait for @awegrzyn and @PascalBoeschoten to give green light to this and then I will let them to disable the MySQL part.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktf We'll get rid of more than just MySQL ;)

@PascalBoeschoten PascalBoeschoten merged commit 612eb62 into AliceO2Group:master Feb 7, 2018
@ktf ktf deleted the fix-lib-common-dependency branch February 7, 2018 14:05
@ktf
Copy link
Member Author

ktf commented Feb 7, 2018

Thanks. Can you tag and do a PR in alidist as well, so that I can continue integrating this in O2?

@PascalBoeschoten
Copy link
Contributor

@ktf Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants