Skip to content

MINIFICPP-1049 - Fix make docker#655

Closed
bakaid wants to merge 1 commit intoapache:masterfrom
bakaid:MINIFICPP-1049
Closed

MINIFICPP-1049 - Fix make docker#655
bakaid wants to merge 1 commit intoapache:masterfrom
bakaid:MINIFICPP-1049

Conversation

@bakaid
Copy link
Copy Markdown
Contributor

@bakaid bakaid commented Sep 30, 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.

Copy link
Copy Markdown
Contributor Author

@bakaid bakaid left a comment

Choose a reason for hiding this comment

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

@apiri This fixed the issue for me, but I would like to wait for @arpadboda's approval to make sure I don't break any of his stuff.

Comment thread docker/DockerBuild.sh
echo "MiNiFi Package: $MINIFI_SOURCE_CODE"

# Copy the MiNiFi source tree to the Docker working directory before building
rm -rf $CMAKE_SOURCE_DIR/docker/minificppsource
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Excluding a path will not delete it in the target, even though we have the --delete flag said (this is the proper behaviour I think). However, this masked the error, because if you have already had the cmake directory copied, it left it as it was, just didn't update it, and the build succeeded. I had to clean up my clone to reproduce the issue.

Comment thread docker/DockerBuild.sh
--exclude '/*_repository*' \
--exclude '/logs' \
--exclude '/cmake' \
--exclude '/cmake-build-*' \
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@arpadboda This was added in #634 - I am not sure what was the original intent here, but this excludes the whole cmake directory, which messes up the whole build process. I changed it to this, based on the .gitignore, but I want to make sure there was no other consideration behind it that I would break with this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wanted to avoid uploading CMakeCache and cmake-build-debug to the container, thanks for fixing.

Comment thread docker/Dockerfile
&& mkdir build \
&& cd build \
&& cmake -DOPENSSL_FORCE_SHARED=true -DDISABLE_JEMALLOC=ON -DSTATIC_BUILD= -DSKIP_TESTS=true -DENABLE_JNI=ON .. \
&& cmake -DDISABLE_JEMALLOC=ON -DSTATIC_BUILD= -DSKIP_TESTS=true -DENABLE_JNI=ON .. \
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OPENSSL_FORCE_SHARED no longer makes sense (doesn't hurt either, but it was confusing).

Copy link
Copy Markdown
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.

Looks good, thanks!

@arpadboda
Copy link
Copy Markdown
Contributor

Executed properly in a new folder on my U18, merging.

@arpadboda arpadboda closed this in b95fc10 Sep 30, 2019
msharee9 pushed a commit to msharee9/nifi-minifi-cpp that referenced this pull request Nov 23, 2019
Signed-off-by: Arpad Boda <aboda@apache.org>

This closes apache#655
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants