-
Notifications
You must be signed in to change notification settings - Fork 22
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
Dependencies update for C# driver. #515
Conversation
PR Review ChecklistDo not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed. Trivial Change
Code
Architecture
|
# along with this program. If not, see <https://www.gnu.org/licenses/>. | ||
# | ||
|
||
# TODO: Generalise and move the same functions (used by java, python, csharp) |
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.
Not sure if it's still relevant. There are some similarities, but such refactoring could make the readability worse. Will think on the deployment stage.
|
||
return [ | ||
DefaultInfo(files = depset([wrap_cxx, wrap_csharp])), | ||
OutputGroupInfo( |
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 is a useful technique to save separate sets of files for future usage. I had troubles with separating C# and CXX files from each other to compile both source and test libraries, and it was a perfect solution!
files = depset([ctx.file.native_lib]), | ||
runfiles = ctx.runfiles(files = [ctx.file.native_lib]) | ||
), | ||
DotnetAssemblyCompileInfo( |
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.
We need to put DotnetAssembly*Info to build our test (specifically BDD test) targets, which require this information for dependencies.
native_lib_name_root = "typedb_driver" | ||
native_lib_name = "{}_native".format(native_lib_name_root) | ||
|
||
# TODO: On Mac, it's enough to pass native_lib_name_root (lib and .dylib are added by the rule) |
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 make an additional PR for it.
srcs = [swig_wrapper_name], | ||
linkshared = True, | ||
linkopts = select({ | ||
# TODO: move http certificate/encryption libraries into arguments |
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 check it out in an additional PR for deployment.
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.
Generally approved provided the couple minor comments are addressed
Looks like you have some unsigned commits. You'll want to amend them with a signature before you merge. |
What is the goal of this PR?
We add all the needed dependencies for building the new C# driver for TypeDB.
What are the changes implemented in this PR?
This PR contains bazel's csharp rules' pulling commands and additional bazel rules for building the native C# library used in the driver and its automatic tests.
TODOs will be completed in a separate PR.
C# driver's PR.
I've put a status: blocked label for explicitness, but I'm not sure what's the correct process, so feel free to correct me.