Initial C++ library#54
Conversation
Add rudimentary Nix shell flake
Language and grammar
seffradev
force-pushed
the
cpp-abstraction
branch
from
81c3d26 to
c4ec32e
Compare
|
@seffradev it would be great if you could merge it with main |
| tc_container_terminate(request_id); | ||
| } | ||
|
|
||
| std::expected<void, std::string> run() { |
There was a problem hiding this comment.
New to this repo but just thought I'd take a look at what this API may look like as I want to consume the library from C++.
Am I right in thinking the use of std::expected here in this header which a user is expected to include, means that the library is only usable from C++23 and above?
If so I think it may be worth reconsidering, many projects including my one are still on C++20 or C++17.
There was a problem hiding this comment.
Good suggestion! If I find the time, I could add the project zeus-cpp/expected with a compilation conditional to either use the polyfill or the standard implementation.
Another thing worth addressing is that I am currently working on an alternative library design using the builder pattern for container creation so future revisions (as this is an early draft) may change.
Thanks for your interest!
There was a problem hiding this comment.
Yes, such an polyfill backport could work.
On my own, I would have probably just used Boost:Outcome, but I am too attached to Boost from the past.
There was a problem hiding this comment.
Re builder pattern - I know the database containers will execute scripts in a standard location but those can be hard to keep in sync with the rest of the system. It would be better to use a standard tool like liquibase or flyway.
My solution was a local driver extension that loaded the database as part of the container's startup sequence. I would have to find the code but I think I used a hook just after the 'is the server ready?' call, but had a small amount of concern that it might be lost if somebody else extended this class and forgot to call 'super.hook()'.
The first lesson from this is that any 'hooks' should be backed by a list of actions instead of a single method. It avoids the risk of extensions forgetting to call 'super.hook()' AND allows developer to replace complex implementations with a series of smaller ones.
The second less is that this is a really valuable extension to the container logic since it frees the developer from any need to explicitly configure the container's database beyond adding a single line to the builder. Flyway loads the files under resources/db/migration/, I don't know what directory liquibase uses. This would need to be configurable though - tests might want to use simplified schemas or preloaded data.
There's a few odd tweaks to this, e.g., if you want to support multiple databases, but they're not common and there are workarounds.
(Think copying localized text from a primary PostgreSQL database into a SQLite database that's then distributed to each host. This reduces the load on the primary database but more importantly makes it impossible for an attacker to "scribble" on the displayed HTML on more than a single server at a time.)
I'll try to find my existing code to clean up and submit as a suggestion. It's clearly not a one-shot - you need to support multiple languages, multiple database migration tools, etc. - but it could be a starting point for these implementations. Plus it might be possible to move the code onto the docker image so the developer would only need to mount the appropriate directory to the container.
There was a problem hiding this comment.
@beargiles I am not sure it was the target repo/thread
oleg-nenashev
left a comment
oleg-nenashev
left a comment
There was a problem hiding this comment.
It would be nice to merge with main after I get #58 over the line.
I do not mind if we merge this experimental codebase first @seffradev . As long as it builds, having a boilerplate in the repo looks reasonable to me, the whole repo is experimental ATM
|
|
||
| add_library(${TARGET} INTERFACE) | ||
|
|
||
| set(CMAKE_CXX_STANDARD 23) |
There was a problem hiding this comment.
FTR I am fine with doing only 23 for now, that's not a blocker
There was a problem hiding this comment.
My biggest argument against merging what we have is that I have a redesign almost done. I just have some UB to refine away.
seffradev
force-pushed
the
cpp-abstraction
branch
2 times, most recently
from
b0f89b5 to
35d2e8c
Compare
|
OK. I am on standby. Let me know once it is ready for review/integration. And thank you! |
|
Perfect. I'm currently fighting the compiler, as I'm using a bit too modern C++ with an even more modern compiler on my machine than what's in the CI. |
seffradev
force-pushed
the
cpp-abstraction
branch
from
35d2e8c to
f691262
Compare
Dev Containersfor the win. We can update it as needed, no need to stick with older versions as of now |
oleg-nenashev
left a comment
oleg-nenashev
left a comment
There was a problem hiding this comment.
+1 for integrating. I can finish packaging on the top of it
seffradev
force-pushed
the
cpp-abstraction
branch
4 times, most recently
from
a441417 to
a88304c
Compare
seffradev
force-pushed
the
cpp-abstraction
branch
from
a88304c to
d1cee04
Compare
|
@seffradev, do you think we can go ahead? I am preparing to work on this repo again during the vacation in July, so it might be great to have all the major code merged into |
|
@oleg-nenashev it's fit for merging! |
|
I will stabilize the PR inside |
4 participants
What does this PR do?
It implements an initial C++ adapter library design.
It depends on #53.
Why is it important?
It provides idiomatic C++, utilizing RAII patterns.
Related issues