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

[scripts] Fix vcpkg_fixup_cmake on non Windows platforms #5630

Merged
merged 7 commits into from
Jun 25, 2019

Conversation

tarcila
Copy link
Contributor

@tarcila tarcila commented Mar 11, 2019

Script was only handling tools executables ending with .exe.
Changed it so anything under /bin/ in transformed.

This fixes for instance FlatcTargets-release.cmake from flatbuffers
port on osx.

Script was only handling tools executables ending with .exe.
Changed it so anything under /bin/ in transformed.

This fixes for instance FlatcTargets-release.cmake from flatbuffers
port on osx.
@Neumann-A
Copy link
Contributor

Have you tested this on Windows? The bin folder contains the *.dlls and *.pdbs on Windows not only the exe. I tried something with CMAKE_EXECUTABLE_SUFFIX within a port a short while ago on windows but it failed du to CMAKE_EXECUTABLE_SUFFIX not being set to .exe for some reason

@tarcila
Copy link
Contributor Author

tarcila commented Mar 11, 2019

I did not test this under Windows for now, as I will only be able to do this later today.

But, I would hope it works as this commit is only about changing references to files inside some
cmake description files, not dealing with the files themselves. And as far as I know those cmake description files only address importable targets -- executables and import libraries (.lib) on Windows, and import lib are not in the bin/ folder.

I'll update the PR when I have tested it

@Neumann-A
Copy link
Contributor

Neumann-A commented Mar 11, 2019

references to files inside some
cmake description files

If those references are pointing to the wrong direction cmake has a problem.

From the VTK Portfile:

if(VTK_WITH_ALL_MODULES)
    file(READ ${CURRENT_PACKAGES_DIR}/share/vtk/VTKTargets-release.cmake VTK_TARGETS_RELEASE_CONTENT)
    string(REPLACE "lib/../XdmfCore.lib" "lib/XdmfCore.lib" VTK_TARGETS_RELEASE_CONTENT "${VTK_TARGETS_RELEASE_CONTENT}")
    string(REPLACE "bin/../XdmfCore.dll" "bin/XdmfCore.dll" VTK_TARGETS_RELEASE_CONTENT "${VTK_TARGETS_RELEASE_CONTENT}")
    string(REPLACE "lib/../vtkxdmf3.lib" "lib/vtkxdmf3.lib" VTK_TARGETS_RELEASE_CONTENT "${VTK_TARGETS_RELEASE_CONTENT}")
    string(REPLACE "bin/../vtkxdmf3.dll" "bin/vtkxdmf3.dll" VTK_TARGETS_RELEASE_CONTENT "${VTK_TARGETS_RELEASE_CONTENT}")
    file(WRITE ${CURRENT_PACKAGES_DIR}/share/vtk/VTKTargets-release.cmake "${VTK_TARGETS_RELEASE_CONTENT}")

    file(READ ${CURRENT_PACKAGES_DIR}/share/vtk/VTKTargets-debug.cmake VTK_TARGETS_DEBUG_CONTENT)
    string(REPLACE "lib/../XdmfCore.lib" "lib/XdmfCore.lib" VTK_TARGETS_DEBUG_CONTENT "${VTK_TARGETS_DEBUG_CONTENT}")
    string(REPLACE "bin/../XdmfCore.dll" "bin/XdmfCore.dll" VTK_TARGETS_DEBUG_CONTENT "${VTK_TARGETS_DEBUG_CONTENT}")
    string(REPLACE "lib/../vtkxdmf3.lib" "lib/vtkxdmf3.lib" VTK_TARGETS_DEBUG_CONTENT "${VTK_TARGETS_DEBUG_CONTENT}")
    string(REPLACE "bin/../vtkxdmf3.dll" "bin/vtkxdmf3.dll" VTK_TARGETS_DEBUG_CONTENT "${VTK_TARGETS_DEBUG_CONTENT}")
    file(WRITE ${CURRENT_PACKAGES_DIR}/share/vtk/VTKTargets-debug.cmake "${VTK_TARGETS_DEBUG_CONTENT}")
endif()

So PCL will probably fail with your commit or some external project with wants to install the vtk dlls will fail to do so. (e.g. not installing the required dlls)

@tarcila
Copy link
Contributor Author

tarcila commented Mar 11, 2019

I confirm that trying to solve the problem with executables on non Windows platforms, my commit does break Windows DLL support in the aforementioned files. I'll submit an updated PR later today.

On Windows platforms use .exe. On other platforms do not use any extension.
@tarcila tarcila force-pushed the update-fix-cmake-target-script branch from 0ee83b7 to 61c4daa Compare March 11, 2019 17:28
@tarcila
Copy link
Contributor Author

tarcila commented Mar 11, 2019

I have pushed an updated version where:

  • only .exe are transformed on Windows platforms (previous behaviour)
  • all files are transformed on other platforms

As of now, I have been able to successfully build pcl on Windows, vtk, harfbuzz on Windows and OSX, resolving regressions introduced by earlier version of the PR.
Now flatbuffers's flatc is correctly referenced by FlatcTargets-release.cmake on OSX,. It also fixes my in progress Capnproto package.

@@ -27,6 +28,13 @@ function(vcpkg_fixup_cmake_targets)
set(_vfct_TARGET_PATH share/${PORT})
endif()


if(NOT VCPKG_CMAKE_SYSTEM_NAME OR VCPKG_CMAKE_SYSTEM_NAME STREQUAL "Windows")
Copy link
Contributor

@Neumann-A Neumann-A Mar 11, 2019

Choose a reason for hiding this comment

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

does this include WindowsStore? (I dont know if WindowsStore is also exe)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not include WindowsStore as I have made a typo: Windows is already tested by the empty VCPKG_CMAKE_SYSTEM_NAME. The second one was intended to be WindowsStore.
Will fix it.

Windows Desktop is already tested as empty VCPKG_CMAKE_SYSTEM_NAME.
@Rastaban Rastaban self-assigned this Mar 14, 2019
tarcila added a commit to tarcila/vcpkg that referenced this pull request Mar 22, 2019
Integration test is failing for now because of microsoft#5630 and microsoft#5635
This at least makes the package function on Windows x86 and x64.
@ras0219-msft
Copy link
Contributor

LGTM

@Rastaban
Copy link
Contributor

Rastaban commented Jun 5, 2019

/azp run

@Rastaban
Copy link
Contributor

Rastaban commented Jun 6, 2019

At preliminary glance at the test results tells me they are probably all unrelated failures (flaky or conflicting ports) I am looking into the failures and will report back anything that looks like it could be valid.

@@ -1,4 +1,4 @@
Source: grpc
Version: 1.21.1
Version: 1.21.1-1
Copy link
Contributor

@Neumann-A Neumann-A Jun 14, 2019

Choose a reason for hiding this comment

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

@Rastaban: If you are already touching grpc you probably also want to move the extra handling of grpc in the toolchain file vcpkg.cmake in the find_package function into a separate vcpkg_cmake_wrapper.cmake.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah already merged nevermind

@Rastaban Rastaban merged commit 54b3be6 into microsoft:master Jun 25, 2019
tarcila added a commit to tarcila/vcpkg that referenced this pull request Jun 25, 2019
Now that microsoft#5630 is fix, this package is fully supported on both Linux
and Mac OS X.
Rastaban pushed a commit that referenced this pull request Jun 25, 2019
Now that #5630 is fix, this package is fully supported on both Linux
and Mac OS X.
pull bot pushed a commit to harrysummer/vcpkg that referenced this pull request Jun 25, 2019
Now that microsoft#5630 is fix, this package is fully supported on both Linux
and Mac OS X.
@tarcila tarcila deleted the update-fix-cmake-target-script branch August 21, 2019 14:02
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.

4 participants