Change to valid C++ namespace identifier

[egorpugin]

No description provided.

[netlify]

✅ Deploy Preview for dpp-dev ready!

Name	Link
🔨 Latest commit	2f8ab87
🔍 Latest deploy log	https://app.netlify.com/sites/dpp-dev/deploys/672f4eed2ffc7300089f85e0
😎 Deploy Preview	https://deploy-preview-1330--dpp-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[braindigitalis]

we don't use this definition, mlspp is an internal dependency and no end user should be including its headers directly. as such this is not a required change.
Not to mention, whenever you rebuild, mlspp's cmakelists will overwrite this file as it is a generated file.

[egorpugin]

Is it possible to apply this fix for other DPP users (out of your intention scope)?

[braindigitalis]

Is it possible to apply this fix for other DPP users (out of your intention scope)?

please read what i put, this is auto generated from namespace.h.in. your changes will be overwritten on running cmake.

Users do not include this file, there is no end user who will use this. It is not even part of include/dpp but is buried in src/mlspp/include. This is not installed.

[egorpugin]

I see. Maybe remove it then?

[Neko-Life]

This is auto configured file from within mlspp CMakelists.txt, if you wanna change the namespace you can pass -DMLS_NAMESPACE_SUFFIX=namespace_name to cmake but only the suffix is configurable, we may need to change it from the CMakelists.txt itself just for the sake of validity as there's no problem ever reported because of this. Also ignore and remove the generated file from the repo

[egorpugin]

@Neko-Life

This makes sense, thank you.
It seems the namespace is incorrect in cmake.

If this namespace.h is indeed generated, is it possible to remove it from repo? It is very confusing as you may see.

[braindigitalis]

but why do you need it to be correct?

end users do not and CANNOT include it?

[Neko-Life]

But why do you need this "fix"? What you wanna do and what kind of problem you were having?

[braindigitalis]

@Neko-Life

This makes sense, thank you. It seems the namespace is incorrect in cmake.

If this namespace.h is indeed generated, is it possible to remove it from repo? It is very confusing as you may see.

its part of mlspp which is a third party dependency.

[egorpugin]

But why do you need this "fix"? What you wanna do and what kind of problem you were having?

My build started using that file since it is in repo. Hence build errors.
If the file is generated I thought most of project do not keep them in the tree.

its part of mlspp which is a third party dependency.

But you've said mlspp is bundled and changed for some needs.
This means it is possible to do some extra polishing.

---

It seems this chunk of code
https://github.com/brainboxdotcc/DPP/blob/master/mlspp/CMakeLists.txt#L19C5-L19C97

if(MLS_NAMESPACE_SUFFIX)
    set(MLS_CXX_NAMESPACE "mls_${MLS_NAMESPACE_SUFFIX}" CACHE STRING "Top-level Namespace for CXX")
    set(MLS_EXPORT_NAMESPACE "MLSPP${MLS_NAMESPACE_SUFFIX}" CACHE STRING "Namespace for CMake Export")
else()
    set(MLS_CXX_NAMESPACE "../include/dpp/mlspp/mls" CACHE STRING "Top-level Namespace for CXX")
    set(MLS_EXPORT_NAMESPACE "MLSPP" CACHE STRING "Namespace for CMake Export")
endif()

will be more correct as (see line #5)

if(MLS_NAMESPACE_SUFFIX)
    set(MLS_CXX_NAMESPACE "mls_${MLS_NAMESPACE_SUFFIX}" CACHE STRING "Top-level Namespace for CXX")
    set(MLS_EXPORT_NAMESPACE "MLSPP${MLS_NAMESPACE_SUFFIX}" CACHE STRING "Namespace for CMake Export")
else()
    set(MLS_CXX_NAMESPACE "mls_dpp" CACHE STRING "Top-level Namespace for CXX")
    set(MLS_EXPORT_NAMESPACE "MLSPP" CACHE STRING "Namespace for CMake Export")
endif()

[braindigitalis]

But why do you need this "fix"? What you wanna do and what kind of problem you were having?

My build started using that file since it is in repo. Hence build errors. If the file is generated I thought most of project do not keep them in the tree.

its part of mlspp which is a third party dependency.

But you've said mlspp is bundled and changed for some needs. This means it is possible to do some extra polishing.

It seems this chunk of code master/mlspp/CMakeLists.txt#L19C5-L19C97

if(MLS_NAMESPACE_SUFFIX)
    set(MLS_CXX_NAMESPACE "mls_${MLS_NAMESPACE_SUFFIX}" CACHE STRING "Top-level Namespace for CXX")
    set(MLS_EXPORT_NAMESPACE "MLSPP${MLS_NAMESPACE_SUFFIX}" CACHE STRING "Namespace for CMake Export")
else()
    set(MLS_CXX_NAMESPACE "../include/dpp/mlspp/mls" CACHE STRING "Top-level Namespace for CXX")
    set(MLS_EXPORT_NAMESPACE "MLSPP" CACHE STRING "Namespace for CMake Export")
endif()

will be more correct as (see line #5)

if(MLS_NAMESPACE_SUFFIX)
    set(MLS_CXX_NAMESPACE "mls_${MLS_NAMESPACE_SUFFIX}" CACHE STRING "Top-level Namespace for CXX")
    set(MLS_EXPORT_NAMESPACE "MLSPP${MLS_NAMESPACE_SUFFIX}" CACHE STRING "Namespace for CMake Export")
else()
    set(MLS_CXX_NAMESPACE "mls_dpp" CACHE STRING "Top-level Namespace for CXX")
    set(MLS_EXPORT_NAMESPACE "MLSPP" CACHE STRING "Namespace for CMake Export")
endif()

no, we do not want to do extra polishing in mlspp. The only thing changed in mlspp was its build scripts so that it will accomadate static linking into dpp, and dpp's built in nlohmann json. mlspp is a security lib, and we are not qualified to polish it.

[braindigitalis]

My build started using that file since it is in repo. Hence build errors. If the file is generated I thought most of project do not keep them in the tree.

but how, and why? what exactly would you be using and including it for, and how did you even include it, considering it isnt in the includes directory of the project? What would you be doing that needs you to
a) include the namespace header
b) instantiate mlspp?

[egorpugin]

no, we do not want to do extra polishing in mlspp. The only thing changed in mlspp was its build scripts so that it will accomadate static linking into dpp, and dpp's built in nlohmann json. mlspp is a security lib, and we are not qualified to polish it.

I see, so fix should go into the upstream then. Checking mlspp.

---

Here is what I've found there.

https://github.com/cisco/mlspp/blob/main/CMakeLists.txt#L19

if(MLS_NAMESPACE_SUFFIX)
    set(MLS_CXX_NAMESPACE "mls_${MLS_NAMESPACE_SUFFIX}" CACHE STRING "Top-level Namespace for CXX")
    set(MLS_EXPORT_NAMESPACE "MLSPP${MLS_NAMESPACE_SUFFIX}" CACHE STRING "Namespace for CMake Export")
else()
    set(MLS_CXX_NAMESPACE "mls" CACHE STRING "Top-level Namespace for CXX")
    set(MLS_EXPORT_NAMESPACE "MLSPP" CACHE STRING "Namespace for CMake Export")
endif()

So, they already fixed what I'm asking (see line # 5 again). It is already set(MLS_CXX_NAMESPACE "mls" instead of incorrect set(MLS_CXX_NAMESPACE "../include/dpp/mlspp/mls".

but how, and why? what exactly would you be using and including it for, and how did you even include it, considering it isnt in the includes directory of the project? What would you be doing that needs you to

I build dpp. Dpp requires (uses) bundled mlspp. I build this bundled mlspp. It uses those version.h and namespace.h in include dir. After that namespace.h contains invalid namespace id that causes errors.

And I see now that this issue is already fixed upstream.
So if you bundled upstream version with that namespace issue, I kindly ask to update this file (and probably CMakeLists.txt) accordingly to fix the issue.

[braindigitalis]

** I build this bundled mlspp.**

why do you build it, when dpp should automatically build it how it wants it? If you build it by hand, youre missing out on some important configuration.

we will backport that fix, but please dont try and just go into mlspp directory and hand build mlspp, if its bundled, its bundled for a reason and shouldnt just be hand built.

[braindigitalis]

closing this pr as we will backport the fix from mlspp instead