[opencascade] fix #16252 by Dejauxvue · Pull Request #16513 · microsoft/vcpkg

Dejauxvue · 2021-03-03T09:40:02Z

fix #16252 by exporting include directories with the targets and replacing internal includes from #include <...> to #include "..." post installation

tested on x64-windows

…h the targets and replacing internal includes from #include "..." to #include<...> post installation

ghost · 2021-03-03T09:40:23Z

All CLA requirements met.

ports/opencascade/portfile.cmake

ports/opencascade/install-include-dir.patch

ports/opencascade/portfile.cmake

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

ports/opencascade/portfile.cmake

NancyLi1013 · 2021-03-09T01:38:50Z

Hi @Dejauxvue

Would you like to add a patch for this changes?

diff --git a/adm/templates/OpenCASCADEConfig.cmake.in b/adm/templates/OpenCASCADEConfig.cmake.in
index 4937103b..cd35e07d 100644
--- a/adm/templates/OpenCASCADEConfig.cmake.in
+++ b/adm/templates/OpenCASCADEConfig.cmake.in
@@ -23,6 +23,7 @@ set (OpenCASCADE_DEVELOPMENT_VERSION "@OCC_VERSION_DEVELOPMENT@")
 # - in Windows style: $INSTALL_DIR/cmake
 get_filename_component (OpenCASCADE_INSTALL_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
 get_filename_component (OpenCASCADE_INSTALL_PREFIX "${OpenCASCADE_INSTALL_PREFIX}" PATH)
+get_filename_component (OpenCASCADE_INSTALL_PREFIX "${OpenCASCADE_INSTALL_PREFIX}" PATH)
 if (OpenCASCADE_INSTALL_PREFIX MATCHES "/cmake$")
   get_filename_component (OpenCASCADE_INSTALL_PREFIX "${OpenCASCADE_INSTALL_PREFIX}" PATH)
 endif()

Since it was removed by PR #15997, but it is still a problem in current version. So the patch is required.

…5997

Dejauxvue · 2021-03-10T09:21:58Z

Hi @Dejauxvue

Would you like to add a patch for this changes?

diff --git a/adm/templates/OpenCASCADEConfig.cmake.in b/adm/templates/OpenCASCADEConfig.cmake.in
index 4937103b..cd35e07d 100644
--- a/adm/templates/OpenCASCADEConfig.cmake.in
+++ b/adm/templates/OpenCASCADEConfig.cmake.in
@@ -23,6 +23,7 @@ set (OpenCASCADE_DEVELOPMENT_VERSION "@OCC_VERSION_DEVELOPMENT@")
 # - in Windows style: $INSTALL_DIR/cmake
 get_filename_component (OpenCASCADE_INSTALL_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
 get_filename_component (OpenCASCADE_INSTALL_PREFIX "${OpenCASCADE_INSTALL_PREFIX}" PATH)
+get_filename_component (OpenCASCADE_INSTALL_PREFIX "${OpenCASCADE_INSTALL_PREFIX}" PATH)
 if (OpenCASCADE_INSTALL_PREFIX MATCHES "/cmake$")
   get_filename_component (OpenCASCADE_INSTALL_PREFIX "${OpenCASCADE_INSTALL_PREFIX}" PATH)
 endif()

Since it was removed by PR #15997, but it is still a problem in current version. So the patch is required.

I tried applying the patch directly in a naive way and it doesn't seem to work.
So I tried to reproduce the changes, but I don't really understand what the patch does.
Does it add the line

get_filename_component (OpenCASCADE_INSTALL_PREFIX "${OpenCASCADE_INSTALL_PREFIX}" PATH)

directly after the same line? Does this even have any effect?

sudo-liljoa · 2021-03-10T14:23:50Z

Hi @Dejauxvue
Would you like to add a patch for this changes?
diff --git a/adm/templates/OpenCASCADEConfig.cmake.in b/adm/templates/OpenCASCADEConfig.cmake.in
index 4937103b..cd35e07d 100644
--- a/adm/templates/OpenCASCADEConfig.cmake.in
+++ b/adm/templates/OpenCASCADEConfig.cmake.in
@@ -23,6 +23,7 @@ set (OpenCASCADE_DEVELOPMENT_VERSION "@OCC_VERSION_DEVELOPMENT@")
 # - in Windows style: $INSTALL_DIR/cmake
 get_filename_component (OpenCASCADE_INSTALL_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
 get_filename_component (OpenCASCADE_INSTALL_PREFIX "${OpenCASCADE_INSTALL_PREFIX}" PATH)
+get_filename_component (OpenCASCADE_INSTALL_PREFIX "${OpenCASCADE_INSTALL_PREFIX}" PATH)
 if (OpenCASCADE_INSTALL_PREFIX MATCHES "/cmake$")
   get_filename_component (OpenCASCADE_INSTALL_PREFIX "${OpenCASCADE_INSTALL_PREFIX}" PATH)
 endif()
Since it was removed by PR #15997, but it is still a problem in current version. So the patch is required.
I tried applying the patch directly in a naive way and it doesn't seem to work.
So I tried to reproduce the changes, but I don't really understand what the patch does.
Does it add the line
get_filename_component (OpenCASCADE_INSTALL_PREFIX "${OpenCASCADE_INSTALL_PREFIX}" PATH)
directly after the same line? Does this even have any effect?

just to clear up, it basicly does a cd .. for the install prefix, causing it to install in the correct prefix path instead of the shared subfolder

ras0219-msft

Thanks for the fixes! This all generally looks great.

However, I notice that the PR doesn't currently include the portfile changes to swap <>s with ""s that you mentioned here. They seem necessary for our MSBuild integration to work.

Could you explain why they aren't needed or add then to this PR?

Dejauxvue · 2021-03-11T11:25:37Z

@ras0219-msft
I had a conversation with @JackBoosY and they suggested that we should add a hint to MSBuild user that they should include the directory manually. (I don't know how to link to a conversation, but you can find it in this thread)

JackBoosY · 2021-03-12T15:25:40Z

@ras0219-msft For such ports that export specific include paths in cmake, since we did not provide these paths to msbuild integration after building them, I think that manually adding these paths to the include path of the header file is the only solution.
Because there are too many relative paths included, it is difficult for us to fully consider the included code.

Dejauxvue · 2021-03-12T15:37:32Z

shouldn't we then install the includes from opencascade directly to include instead of include/opencascade ?
If adding include/opencascade to the include paths anyways, I don't think, there's much benefit from encapsulating the headers in the include/opencascade subdirectory

JackBoosY · 2021-03-12T15:58:34Z

@Dejauxvue We will have a internal discussion.

JackBoosY · 2021-03-16T02:40:25Z

@Dejauxvue After some discussion, we think that your previous modification to the path in portfile.cmake is correct, please restore it. Sorry.

Dejauxvue · 2021-03-16T10:43:24Z

@JackBoosY
No problem, I'll do it later or tomorrow.
Btw, is there a way, I can test installation of a package without rebuilding it every time?
It takes me about 1h each time to find a bug in the post-build portion of the port file

JackBoosY · 2021-03-16T15:10:58Z

@Dejauxvue I wish too but not. Sorry.

….> to #include "..."" also refined regex to allow white spaces between # and 'include' This reverts commit 4b36273.

ras0219-msft

This looks great to me, thanks for the hard work here!

Just to double check: you've tested this on x64-windows via CMake; have you tested it via MSBuild? Does it work "out of the box" or is the extra include path required for normal usage?

Dejauxvue · 2021-03-17T09:21:22Z

@ras0219-msft
Thank you for the heads-up!
There are indeed two things that should be adressed:

the exported include directory is include/opencascade for cmake. This means that a project configured by cmake needs to include occt headers by #include <gp.hxx>, while code being configured by importing vcpkg.targets into a Visual Studio project needs to include headers by #include <opencascade/gp.hxx>.
I suggest to change the exported include directory to include instead of opencascade/include so that #include <opencascade/gp.hxx> is always the right way
Secondly, when configuring with CMake, The compilation works fine, however the linker does cannot resolve all symbols because some targets don't get exported. At this point, I don't know why this happens, but I'll look into it.

In Summary: MSBuild works; CMake uses different include conventions and not all targets get exported

Edit: I applied my suggested changes for the first issue. The second issue does not seem to be related to my changes, I think. As I did not change what targets get exported. Can someone confirm this?

Edit2: the second issue is probably not an issue, I just did not link my example to all opencascade targets

Edit3: can confirm that the cmake linker error was wrong usage; Tested MSBuild an CMake with the following source code and it compiles and links successfully
occt_test_cmake.zip

…ade to include

ras0219-msft · 2021-03-22T22:19:52Z

Yep, this looks like the right solution to me, thanks for the continued improvement!

Just to note specifically in case it wasn't mentioned here: the CMake change adds the root include/ as well, so users can include via either method (prefix with opencascade/ or direct names).

[opencascade] fix microsoft#16252 by exporting includ directories wit…

b4a11b4

…h the targets and replacing internal includes from #include "..." to #include<...> post installation

JoBloess added 2 commits March 3, 2021 12:47

[opencascade] added port version

9115c82

[opencascade] update SHA of versions/o-/opencascade.json

4e734b0

Cheney-W assigned NancyLi1013 Mar 3, 2021

NancyLi1013 added the category:port-bug The issue is with a library, which is something the port should already support label Mar 4, 2021

NancyLi1013 suggested changes Mar 4, 2021

View reviewed changes

ports/opencascade/portfile.cmake Outdated Show resolved Hide resolved

ports/opencascade/portfile.cmake Outdated Show resolved Hide resolved

NancyLi1013 suggested changes Mar 4, 2021

View reviewed changes

ports/opencascade/install-include-dir.patch Outdated Show resolved Hide resolved

ports/opencascade/portfile.cmake Outdated Show resolved Hide resolved

ports/opencascade/portfile.cmake Outdated Show resolved Hide resolved

NancyLi1013 added the requires:author-response label Mar 4, 2021

Dejauxvue and others added 3 commits March 4, 2021 08:23

Update ports/opencascade/portfile.cmake

d7065a5

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

Update ports/opencascade/portfile.cmake

42711c0

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

Update ports/opencascade/portfile.cmake

e1e7f23

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

JackBoosY suggested changes Mar 4, 2021

View reviewed changes

ports/opencascade/portfile.cmake Show resolved Hide resolved

JoBloess added 2 commits March 4, 2021 12:18

remove commented line for BUILD_INTERFACE

c334c0c

updated version

9e42e5b

JackBoosY removed the requires:author-response label Mar 4, 2021

JoBloess added 2 commits March 5, 2021 10:41

remove replacement of includes form #include <...> to #include "..."

4b36273

update port version

3762d7a

JackBoosY approved these changes Mar 5, 2021

View reviewed changes

JackBoosY marked this pull request as ready for review March 5, 2021 10:07

JackBoosY requested a review from NancyLi1013 March 5, 2021 10:07

NancyLi1013 approved these changes Mar 5, 2021

View reviewed changes

NancyLi1013 added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Mar 5, 2021

NancyLi1013 mentioned this pull request Mar 9, 2021

[opencascade] Fix OpenCASCADE_INSTALL_PREFIX path in config #15056

Merged

NancyLi1013 added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Mar 9, 2021

added fix-install-prefix-path.patch, which was removed by microsoft#1…

c9bab16

…5997

fix fix-install-prefix-path.patch

cb2b122

update port version

f1c22a9

JackBoosY requested a review from NancyLi1013 March 10, 2021 12:02

JackBoosY removed the requires:author-response label Mar 10, 2021

ras0219-msft reviewed Mar 10, 2021

View reviewed changes

NancyLi1013 added the requires:author-response label Mar 11, 2021

Dejauxvue requested a review from JackBoosY March 11, 2021 14:26

JackBoosY approved these changes Mar 12, 2021

View reviewed changes

JackBoosY added requires:discussion and removed requires:author-response labels Mar 12, 2021

JackBoosY removed the requires:discussion label Mar 16, 2021

JackBoosY added the requires:author-response label Mar 16, 2021

JoBloess added 2 commits March 16, 2021 17:12

[opencascade]Revert "remove replacement of includes form #include <..…

5ed536b

….> to #include "..."" also refined regex to allow white spaces between # and 'include' This reverts commit 4b36273.

[opencascade] update port version

983abab

ras0219-msft approved these changes Mar 16, 2021

View reviewed changes

JackBoosY approved these changes Mar 17, 2021

View reviewed changes

JoBloess added 2 commits March 17, 2021 10:28

[opencascade] change exported include directory from include/opencasc…

8e923e0

…ade to include

[opencascade] update port version

48233ee

JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Mar 22, 2021

ras0219-msft merged commit c8021b4 into microsoft:master Mar 22, 2021

Conversation

Dejauxvue commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 3, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NancyLi1013 commented Mar 9, 2021

Uh oh!

Dejauxvue commented Mar 10, 2021

Uh oh!

sudo-liljoa commented Mar 10, 2021

Uh oh!

ras0219-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Dejauxvue commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackBoosY commented Mar 12, 2021

Uh oh!

Dejauxvue commented Mar 12, 2021

Uh oh!

JackBoosY commented Mar 12, 2021

Uh oh!

JackBoosY commented Mar 16, 2021

Uh oh!

Dejauxvue commented Mar 16, 2021

Uh oh!

JackBoosY commented Mar 16, 2021

Uh oh!

ras0219-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Dejauxvue commented Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ras0219-msft commented Mar 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Dejauxvue commented Mar 3, 2021 •

edited

Loading

ghost commented Mar 3, 2021 •

edited by ghost

Loading

Dejauxvue commented Mar 11, 2021 •

edited

Loading

Dejauxvue commented Mar 17, 2021 •

edited

Loading