-
Notifications
You must be signed in to change notification settings - Fork 111
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
Add support for GnuTLS #663
base: master
Are you sure you want to change the base?
Changes from 17 commits
23fed28
49561d2
ad9271b
7444c04
9d94ec5
6ef17e8
cc9d4d8
1367a85
8b9ee52
d3778b3
6eb8d09
52ef72d
10e6871
0aa3288
abf1536
5746ffd
a9da0f2
ac1ecbb
195e1f9
50688b7
d5a3ef2
dc98256
c0ea73c
11bc857
9867f96
3a4a8e0
a6b4579
a018a30
c7d4bb6
bf54b28
98c658f
36fc136
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,7 @@ | ||
CMakeLists.txt.user* | ||
|
||
# Build artifacts | ||
build/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, don't add this. Builds should not happen inside the source tree. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The repository's ReadMe suggests doing just that. I've seen several other projects follow the same format. What's so wrong with adding the build/ directory to this file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the very least, if it's going to be added to the repository, it needs to happen in it's own pull request. There are a variety of reasons why builds should not be done inside the source directory, but one specific one that I'll share to help you research the topic, is that cmake natively understands multiple configurations. If you put the build inside the source directory, you pre-clude the ability to have multiple build configurations at the same time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can remove it. It's just that it was making my life easier as I've always built libraries this way. |
||
|
||
# VS code settings | ||
.vscode/ |
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 isn't right
You're overwriting the flags that are already selected for the "1" case.
If you want to add the GNUTLS implementation to the unit tests, you'll need to add more items to the list "matrix.pattern", and not reuse them.
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.
My recommendation is to shrink the text matrix a bit.
Right now we have "MQTT_TEST_1" through MQTT_TEST_7". Most likely these can be merged down into a smaller list, perhaps 1 though 5 or something. That'll keep the combinatorial explosion slightly under control.
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 can reduce the test list but I wasn't sure which ones are okay to remove. If you and @redboltz both agree to keep it 1-5 only, I can easily update it
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 think you might have misunderstood.
My recommendation was to re-arrange the MQTT_TEST_1 and so on binaries so that the code for each of them is preserved, and all the tests continue working, but there are fewer build varients.
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.
Sorry, not sure I'm fully following your recommendation.
Did you mean rearranging the patterns such that they are ordered in a way where all patterns that have MQTT_TEST_1
enabled are one after another?
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.
Specifically I meant changing text/CMakeLists.txt to have the different MQTT_TEST_# entries have different cpp files included in them.