Skip to content
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

MINIFICPP-1022 - Refactored third party build system #661

Closed
wants to merge 5 commits into from

Conversation

bakaid
Copy link
Contributor

@bakaid bakaid commented Oct 9, 2019

Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

  • Has your PR been rebased against the latest commit within the target branch (typically master)?

  • Is your initial contribution a single, squashed commit?

For code changes:

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file?
  • If applicable, have you updated the NOTICE file?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

@bakaid
Copy link
Contributor Author

bakaid commented Oct 9, 2019

Please read https://github.com/bakaid/nifi-minifi-cpp/blob/MINIFICPP-1022/ThirdParties.md before reviewing other parts of this PR.

I still have to run tests to make sure I didn't break anything, including JNI and python.

@phrocker
Copy link
Contributor

I like the idea of this PR for potentially 0.8 or 1.x, but I haven't resolved that in my head. I think the bigger thing is that perhaps we could move the third parties readme to a contribution folder and integrate it as a link in the contrib guide. Is there a way we could automate checks to for this?

README.md Outdated
@@ -530,6 +530,10 @@ On Windows it is suggested that MSI be used for installation.

Please see [Extensions.md](Extensions.md) on how to build and run conditionally built dependencies and extensions.

### Third parties

Please see [ThirdParties.md](ThirdParties.md) on how MiNiFi builds and uses third party libraries and how you can add new ones.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems more developer focused, perhaps this could be split into the contrib guide?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's fair, moved the reference to CONTRIB.md

Copy link
Contributor

@arpadboda arpadboda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Far from being done, some minor comments.

ThirdParties.md Outdated Show resolved Hide resolved
```
add_library(foo INTERFACE)
target_include_directories(foo INTERFACE "${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/libfoo-1.0.0")
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely written, cool doc, I like it!

cmake/BuildTests.cmake Outdated Show resolved Hide resolved
cmake/BundledAwsSdkCpp.cmake Show resolved Hide resolved
cmake/BundledAwsSdkCpp.cmake Show resolved Hide resolved
cmake/BundledCivetWeb.cmake Show resolved Hide resolved
Copy link
Contributor

@arpadboda arpadboda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through the doc, looks good overall, some minor comments added.

ThirdParties.md Outdated Show resolved Hide resolved
ThirdParties.md Outdated Show resolved Hide resolved
ThirdParties.md Outdated Show resolved Hide resolved
Copy link
Contributor

@arpadboda arpadboda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished reviewing cmake changes, they look good!

Added some minor comments, but no red flags were spotted.

cmake/BundledLibArchive.cmake Show resolved Hide resolved
cmake/BundledLibCOAP.cmake Show resolved Hide resolved
cmake/BundledLibRdKafka.cmake Outdated Show resolved Hide resolved
cmake/BundledPahoMqttC.cmake Show resolved Hide resolved
# specific language governing permissions and limitations
# under the License.

function(target_wholearchive_library TARGET ITEM)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May god bless you for removing all the copy-paste occurrences of this! 🙏

@bakaid
Copy link
Contributor Author

bakaid commented Jan 13, 2020

@arpadboda Addressed your review comments, I will start a final round of verification and self-review.

Copy link
Contributor

@arpadboda arpadboda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and seems to work for me on Debian, Win and Mac.

@bakaid
Copy link
Contributor Author

bakaid commented Jan 13, 2020

@arpadboda macOS failure on Travis is an unrelated timeout at the very end, I've finished my verification, I'll merge soon.

@bakaid bakaid closed this in 6e2f859 Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants