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.

Sign up for GitHub

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

Jump to bottom

MINIFICPP-1099 - Fix Windows MSI build #692

Closed
wants to merge 2 commits into from
Closed

MINIFICPP-1099 - Fix Windows MSI build #692

wants to merge 2 commits into from

Conversation

bakaid
Copy link
Contributor

@bakaid bakaid commented Dec 10, 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 Dec 10, 2019

Still testing different configurations - @amarmer if you could try it with your on environment, that would be nice.

bakaid
bakaid commented Dec 10, 2019
View reviewed changes
CMakeLists.txt
# Determine the path of the VC Redistributable Merge Modules
if (DEFINED ENV{VCToolsRedistDir})
# Just get the redist dir that has been set by the build environment
set(VCRUNTIME_REDIST_DIR $ENV{VCToolsRedistDir})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like a pretty reliable method with 2017, it is both defined when building directly from Visual Studio, and when using a terminal vcvarsall.bat, but I included the backup method because I am not sure how reliable it is across VS versions.

bakaid
bakaid commented Dec 10, 2019
View reviewed changes
win_build_vs.bat
@@ -23,6 +23,7 @@ set builddir=%1
set skiptests=OFF
set cmake_build_type=Release
set build_type=Release
set build_platform=Win32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using with vcvarsall.bat, it sets an environment variable which makes msbuild default to x86 as the default platform, not Win32, so we use this to make sure we use the proper Win32 platform.

arpadboda
arpadboda reviewed Dec 10, 2019
View reviewed changes
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.

Looks good at first, will test.

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@am-c-p-p
Copy link

am-c-p-p commented Dec 11, 2019 •
edited
Loading

I tested. LGTM!
It would be good to test minifi.exe (without using installer) on machine with old redistributables, (maybe on Windows 7) and verify that it doesn't work and then install it and check that it works.

@bakaid
Copy link
Contributor Author

bakaid commented Dec 11, 2019

I tested. LGTM!
It would be good to test minifi.exe (without using installer) on machine with old redistributables, (maybe on Windows 7) and verify that it doesn't work and then install it and check that it works.

@amarmer Thank you for trying it.
I have tested it on a Windows 7 instance, and I verified that if I had a preinstalled VC 2015 Runtime, our msi really did update the dlls to a newer version.
I couldn't try it from a not working state, as the older DLLs are still binary compatible, even if it is unsupported to use an older version than our code was built with - this is the reason we have been able to run on Windows at all so far.

@arpadboda
Copy link
Contributor

arpadboda commented Dec 12, 2019 •
edited
Loading

Tested the build on Win10 X64 with VS2017 community, builds as expected, installer works, MiNiFi starts after installation.

The CI failure seems to be unrelated, merging this.

@arpadboda arpadboda closed this in 2eb70d9 Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arpadboda arpadboda arpadboda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@bakaid @am-c-p-p @arpadboda