-
Notifications
You must be signed in to change notification settings - Fork 126
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
new class dai::Path
for APIs that accept path/filenames
#384
Conversation
b0fd32f
to
09f4241
Compare
I think in any implementation approach, we have to deal with the path charset when it is displayed. throw std::runtime_error(fmt::format("Cannot load blob, file at path {} doesn't exist.", path)); That code will fail to compile if code attempts to give a Unicode (wchar_t) string/path on Windows to
throw std::runtime_error(fmt::format(L"Cannot load blob, file at path {} doesn't exist.", path)); But then the result from I believe there has to be depthai-core specific handler for the switching between
|
Easy code for a custom What changes to depthai-core APIs? Change the param type on the function signatures from I'm still considering is what part of the underlying code renders the replacement text
I think the choice is how much does Luxonis want I very slightly prefer the 2nd alternative approach so to prepare everything now for the future. |
7e202d7
to
5b0da31
Compare
include/depthai/utility/Path.hpp
Outdated
|
||
// TODO C++20 char8_t | ||
// TODO test if caller works when replace "dai::Path" -> "std::filesystem::path" | ||
// TODO test if can `using Path = std::filesystem::path` on C++17 to completely use STL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 - maybe we do that ifdef here and create another c++14 vs c++17 compilation matrix entry, to compare.
To reduce the testing load we could for starters do the specific scenario with the test below:
- dynamic lib / static lib
- c++14 / c++17
And then consume the lib, compile the test and run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To date, depthai-core forces compile depthai-core as C++14
Lines 244 to 247 in e03be4c
# Set compiler features (c++14), and disables extensions (g++14) | |
set_property(TARGET ${TARGET_CORE_NAME} PROPERTY CXX_STANDARD 14) | |
set_property(TARGET ${TARGET_CORE_NAME} PROPERTY CXX_STANDARD_REQUIRED ON) | |
set_property(TARGET ${TARGET_CORE_NAME} PROPERTY CXX_EXTENSIONS OFF) |
It is not possible to change to c++17 without someone changing the build or code of depthai-core itself. I don't even do that. 😉 While it is technically possible to ?attempt? a c++17 compile...I (personally) wouldn't prioritize that at this time. There are enough other things to resolve with the SDK.
In contrast, it is very likely there are apps (like mine) that compile themselves as C++17 and link to the c++14 compiled depthai-core. This is the scenario I think is valuable for the testing matrix. I'm already looking forward to the c++20 apps due to c++20's utf-8 handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest commit added optional param to dai_add_test()
where one can declare the desired c++ standard level for the test. It defaults to c++14 like the toplevel depthai-core target. If current toolkit/chain doesn't support the requested std level, it will output status message and skip that test.
include/depthai/utility/Path.hpp
Outdated
#include <spdlog/fmt/bundled/format.h> | ||
template <> | ||
struct fmt::formatter<dai::Path> : formatter<std::string> { | ||
// https://fmt.dev/latest/api.html#formatting-user-defined-types | ||
// https://fmt.dev/latest/syntax.html#format-specification-mini-language | ||
/* | ||
char presentation = 's'; | ||
constexpr auto parse(format_parse_context& ctx) { | ||
auto it = ctx.begin(); | ||
auto end = ctx.end(); | ||
if(it != end && (*it == 's')) presentation = *it++; | ||
if(it != end && *it != '}') throw format_error("invalid format"); | ||
return it; | ||
} | ||
|
||
template <typename FormatContext> | ||
auto format(const dai::Path& p, FormatContext& ctx) -> decltype(ctx.out()) { | ||
return format_to(ctx.out(), "{}", p.string()); | ||
} | ||
*/ | ||
template <typename FormatContext> | ||
auto format(const dai::Path& p, FormatContext& ctx) { | ||
return formatter<std::string>::format(p.string(), ctx); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spdlog is private dependency - this must be hidden in src/utility/Path[...].hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I split the header into two parts. Public header in include tree. Private header in src tree.
I cmake config, build, installed and used that installation successfully with my app.
include/depthai/utility/Path.hpp
Outdated
Path& operator=(const Path&) = default; | ||
Path& operator=(Path&&) = default; | ||
|
||
Path(string_type&& source) noexcept : nativePath(std::move(source)) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I assume will nicely do Python -> C++ integration/conversion. We'd still have to either:
- Create a wrapper in bindings that takes std::string and then calls this
- Or bind and convience pybind that it also implicitly converts python strings to dai::Path
Eitherway, it'll be easy and small changes required there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. If your python code outputs a std::string in its binding, this should work with no python changes.
If not, then it should be a code change of a few characters to call the binding api that outputs a std::string.
5b0da31
to
fb71366
Compare
New force push enables c++17 std::filesystem support. Works ok in my app and in this single tests/filesystem_test. 👎The test+example suite is too long now due to multiplying with conform + poe. |
today added dai::Path to dai::Device, DeviceBase, Resources. One highlight: the I resolved this with a template to specifically check the usb2Mode param type = I added more test cases. It still passes the full test+example suite. I saw the matrix found some issues on a Linux build, I'll get to it soon. |
@themarpe, does your team intend to have duplicate constructors in
My last commit has a bug in it. While fixing, I noticed this dup and since I need to fix one of these constructors, I have to also fix the dups. 🤔 If it works, can I remove the dups? Or should I leave them. |
Hi, please run the CI on this. 3f53255 completes all intended work and time to shift towards bugfix, review feedback. |
c6f08a6
to
ad8a1f2
Compare
- to enable display/log of Path
- enable c++17 std::filesystem support and test cases - split header into public/private parts - cmake for test cases now supports optional c++ standard level param
- add COMPILER_SUPPORTS_CXX{14,17,20,23} vars to Flags.cmake and can be used everywhere
- simplify Device, DeviceBase constructors by delegating - add is_same<> template on constructors with bool param to prevent implicit convert of almost everything to bool - make two DeviceInfo constructors explicit to prevent their use in implicit conversion - relevant test cases - fix minor throw text bugs
- plus test cases
- plus test cases
- fixes luxonis/depthai-core/issues#408
- unable to fully test due to bug luxonis#407
ad8a1f2
to
22cae60
Compare
rebased on develop. Would you please run CI again? |
Sorry for the delay @diablodale |
Will give it a more thorough check in the upcoming days - otherwise looks good on glance |
There is a now/future API concern outstanding. When a std::string (or char*) is given to
This is true also on Windows. Windows will not do any significant conversion. Looking the MSVC STL itself, I can see it does only _Convert_narrow_to_wide(__std_fs_code_page(), _Input); ...meaning it just converts the current Windows codepage (legacy windows tech) from 8-bits into 16-bits. It does not do a utf-8 conversion. So...there is not 100% compatibility of std::filesystem::path::path(std::string);
dai::path::path(std::string);
std::string myutf8 = "🎆🧵👓"
// correct on utf-8 systems like Linux
// garbage on Windows
auto stdPath = std::filesystem::path(myutf8);
// correct on Linux and Windows
auto daiPath = dai::path(myutf8); I would be ok keeping dai::path through c++17 and c++20. It is thin and already uses+converts automatically between std::fs::path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind opening a PR against depthai-python
develop
as well? Just so we can run the CI & compile with bindings in place as well
include/depthai/utility/Path.hpp
Outdated
#pragma once | ||
#if(__cplusplus >= 201703L) || (_MSVC_LANG >= 201703L) | ||
#include <filesystem> | ||
#define DAI_NODISCARD [[nodiscard]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define DAI_NODISCARD [[nodiscard]] | |
#define DEPTHAI_NODISCARD [[nodiscard]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Will do macro change.
- I would also like to squash, but haven't so you can review steps/evolution of this PR. When you are ready, I'll squash and forcepush.
- The bindings I am confident we can get working, but I have never written code in the binding+python space so I'm lacking experience about what exactly to do, different aspects to consider, etc. I suspect one or two places will need an explicit call for python to output its object as a string. Or maybe a user-def type conv (or path constructor) that takes the python object -> dai::path. How "bindings" work...I'm inexperienced 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also like to squash, but haven't so you can review steps/evolution of this PR. When you are ready, I'll squash and forcepush.
Thanks - I'll recheck later today a tad more, but its looking good already
The bindings I am confident we can get working, but I have never written code in the binding+python space so I'm lacking experience about what exactly to do, different aspects to consider, etc. I suspect one or two places will need an explicit call for python to output its object as a string. Or maybe a user-def type conv (or path constructor) that takes the python object -> dai::path. How "bindings" work...I'm inexperienced thinking
If you want, you may open the PR and give me push permissions to it, and I'll do any changes so that it works together with this PR. Hopefully there won't be any changes needed back to core, but it might happen. (also in this case, push permissions back to this PR or I'll post back some "suggested changes")
I think a python obj -> dai::Path will be the cleanest, as it will retain the existing mechanisms of binding the functions (without having to cast to string first, for dai::Path to then do the conversion).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An update. I'm not going to have time to write code for the depthai-python
repro. Python isn't my target, focus, or knowledge.
I burned all of today trying to get depthai-python cmake config working. Mypy still isn't found so no stubs. Compile quickly fails. I predict large churn like I had to do for depthai-core
. I don't have a week just to get the cmake working and then tweak a few bindings that someone with binding experience/setup can likely do in an hour.
I will 100% do the code in depthai-core
. If someone finds a binding that doesn't work and needs depthai-core changes...tell me and I'll write/fix the code of this PR. 💯
I read https://pybind11.readthedocs.io/en/stable/advanced/cast/strings.html#passing-python-strings-to-c and that doc writes the bindings will automatically work if the signature accepts std::string. The dai::Path class has that signature so if the bindings do what their docs write...this should be no code change. 😂 Riiight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, no issue - I'll open one and make any (if) necessary bindings changes.
- review feedback
Have the branch for |
} | ||
|
||
private: | ||
string_type _nativePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string_type _nativePath; | |
string_type nativePath; |
Nitpick - unless it follows some other source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it for usability and to distinguish it from std::filesystem::native()
.
dai::path::_nativePath
is a private member which no dev can access. Some coding tools (like vscode) still show private members in their "intellisense". As I was writing/testing, I found myself often trying to use nativePath
because it was listed and similar to std::fs::native
. I considered changing to a different noun but I couldn't come up with another word that worked as well. So I used an underscore which completely separates it in Intellisense and visually suggests this is something internal/temp.
I don't recommend nativePath
. Do you have another noun you prefer? It can NOT be: native, path, directory, nativePath, nativeDirectory, etc. I think it could be "folder" but paths are not just folders but are also filenames 😝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pathway, trail?
Looked at your dai-python commit. That would have taken me a loong time. Lots of search/replace but I can discern knowing where to replace and some special handling is needed. 🙏Thank you for partnering on that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @diablodale
Sorry for the delay. Is there anything else you've wanted to do before merge?
Otherwise I'll merge tomorrow /w accompanying python bindings changes:)
(And thanks a lot for this effort!)
Something before merge? If you want that, just ask me here. Otherwise, I have no changes and ok to merge. |
Thats okay, I'll squash merge here 👍 |
This is initial concept to fix #352
DO NOT MERGE IT IS NOT READY!
🤔I request feedback on approach, code, everything.
Approach
A single class
dai::Path
. The first two below are strongly influenced by c++17'sstd::filesystem::path
.I used a header because its a simple class, no significant code, and encourages the compilers to inline and/or eliminate codepaths when there is no conversion work to do; this could be the case on posix utf-8 systems.
Not yet done/tested
std::filesystem::path
to the APIs that acceptdai::Path
when the binary behind the APIs was compiled as C++14. I'm hoping it works (or close to working) since all the code is in the header, and the only class member storage is a simple std::string/wstring.dai::Path
instead of the typicalstd::string
.