Skip to content

[master < T0814-MG] Fix CentOS 7 build by top-level memgraph namespace#361

Merged
antaljanosbenjamin merged 6 commits intomasterfrom
T0814-MG-use-boost-SSL
Mar 14, 2022
Merged

[master < T0814-MG] Fix CentOS 7 build by top-level memgraph namespace#361
antaljanosbenjamin merged 6 commits intomasterfrom
T0814-MG-use-boost-SSL

Conversation

@jbajic
Copy link
Contributor

@jbajic jbajic commented Feb 24, 2022

The issue is with the openssl version API compactibility, which was solved by using boost socket. That caused an issue with our query namespace and that's why we are making a huge refactoring.
[master < Task] PR

  • Check, and update documentation if necessary
  • Update changelog

@jbajic jbajic self-assigned this Feb 25, 2022
@jbajic jbajic force-pushed the T0814-MG-use-boost-SSL branch from 26d2240 to 78ae671 Compare February 25, 2022 10:46
gitbuda
gitbuda reviewed Feb 28, 2022
View reviewed changes
Copy link
Member

@gitbuda gitbuda left a comment

Choose a reason for hiding this comment

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

A bunch of heavy lifting here 💪 Very nice!

In addition:

  • I would rename this PR into something like: "Fix CentOS 7 build by top-level memgraph namespace" (it's a bit longer but tells about the whole scope).
  • Make a page under Memgraph Internals explaining why the namespace is important and how to deal with the namespaces correctly (if there is anything specific that contributors have to know).

@jbajic jbajic force-pushed the T0814-MG-use-boost-SSL branch from 9e3ad9b to 369536a Compare March 1, 2022 09:21
@jbajic jbajic changed the title [master < T0814-MG] Fix Centos 7 build [master < T0814-MG] Fix CentOS 7 build by top-level memgraph namespace Mar 1, 2022
@jbajic jbajic changed the title [master < T0814-MG] Fix CentOS 7 build by top-level memgraph namespace [master < T0814-MG] Fix CentOS 7 build by top-level memgraph namespace Mar 1, 2022
@jbajic jbajic force-pushed the T0814-MG-use-boost-SSL branch 7 times, most recently from 7057bf5 to 19647d1 Compare March 3, 2022 09:30
jbajic
jbajic commented Mar 3, 2022
View reviewed changes
@jbajic jbajic marked this pull request as ready for review March 3, 2022 09:44
@jbajic jbajic requested a review from gitbuda March 3, 2022 09:44
kostasrim
kostasrim reviewed Mar 4, 2022
View reviewed changes
Copy link
Contributor

@kostasrim kostasrim left a comment

Choose a reason for hiding this comment

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

@jbajic I am very sorry to point this now but maybe you can sed the change quickly. memgraph is a very big name to warp up the currently existing namespaces and I would suggest renaming it to mg. This will solve the problem we have with centos without introducing the verbosity of the full name.

@jbajic
Copy link
Contributor Author

jbajic commented Mar 4, 2022

@jbajic I am very sorry to point this now but maybe you can sed the change quickly. memgraph is a very big name to warp up the currently existing namespaces and I would suggest renaming it to mg. This will solve the problem we have with centos without introducing the verbosity of the full name.

After discussing internally we have decided to stick with memgraph namespace. You can read more about it here => https://www.notion.so/memgraph/C-Code-Conventions-7497766e0e1647b987733b9e94171423#4f614c6605144610ab9512ac6c15c821

@jbajic jbajic requested a review from kostasrim March 4, 2022 15:04
antaljanosbenjamin
antaljanosbenjamin suggested changes Mar 8, 2022
View reviewed changes
Copy link
Contributor

@antaljanosbenjamin antaljanosbenjamin left a comment

Choose a reason for hiding this comment

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

Nice work Jure! Where I commented "Not necessary" there please check the whole file for extra memgraph:: qualifiers. If you think I asked too much, let me know and I can jump on fixing them.

antaljanosbenjamin
antaljanosbenjamin suggested changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@antaljanosbenjamin antaljanosbenjamin left a comment

Choose a reason for hiding this comment

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

Two small fixes and it is good to go!

@jbajic jbajic force-pushed the T0814-MG-use-boost-SSL branch from 1180edd to 3c559a9 Compare March 11, 2022 12:39
@antaljanosbenjamin
Copy link
Contributor

antaljanosbenjamin commented Mar 11, 2022

I compared the changes before and after the force-push and they were the same, so I don't wait for another review. I will also merge it without squashing to separate the changes of adding new namespace and introducing boost.

@antaljanosbenjamin antaljanosbenjamin merged commit 60ad05a into master Mar 14, 2022
@antaljanosbenjamin antaljanosbenjamin deleted the T0814-MG-use-boost-SSL branch March 14, 2022 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gitbuda gitbuda Awaiting requested review from gitbuda

2 more reviewers

@antaljanosbenjamin antaljanosbenjamin antaljanosbenjamin approved these changes

@kostasrim kostasrim kostasrim approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jbajic jbajic

Labels

tech debt

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jbajic @antaljanosbenjamin @gitbuda @kostasrim