Skip to content

Conversation

RDCH106
Copy link
Contributor

@RDCH106 RDCH106 commented Oct 5, 2020

RBDL - Rigid Body Dynamics Library

  • What does your PR fix?

    • This PR adds a new port for the RDBL for Windows and Linux. It builds the library downloading it from official repository and making compilation through CMake according to Packaging Zipfiles guide.

    • The port only builds the core library and not the optional addons (future work for features).

    • Port was tested in vcpkg 2020.07 with RBDL 2.6.0

  • Which triplets are supported/not supported?

    • Expected supported triplets: Triplets tested by CI baseline
  • Have you updated the CI baseline?

    • Not updated the CI baseline.
  • Does your PR follow the maintainer guide?

@ghost
Copy link

ghost commented Oct 5, 2020

CLA assistant check
All CLA requirements met.

@RDCH106 RDCH106 marked this pull request as ready for review October 6, 2020 11:56
@NancyLi1013 NancyLi1013 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Oct 9, 2020
RDCH106 and others added 6 commits October 9, 2020 10:19
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
RDCH106 and others added 3 commits October 9, 2020 11:08
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
@RDCH106
Copy link
Contributor Author

RDCH106 commented Oct 9, 2020

I am going to update Supports field. It works perfectly for Windows and I am using on Windows.
Linux and OSX remains as future work. ok?

@RDCH106
Copy link
Contributor Author

RDCH106 commented Oct 9, 2020

I don't understand... 🤔

The previous build was: https://dev.azure.com/vcpkg/public/_build/results?buildId=43808&view=logs&j=d3670d0c-cc82-50c9-762c-eccde036829f

And the new one: https://dev.azure.com/vcpkg/public/_build/results?buildId=43818&view=logs&j=a83b7983-e539-5449-9e09-337ff5f52283

  • Between 2 builds starts failing x86-windows when in the previous one was marked as success
  • I exclude linux and osx in Supports field and using vcpkg_fail_port_install... they continue failing in CI

@NancyLi1013 any idea or recommendation about that?

Thanks in advance.

@RDCH106 RDCH106 requested a review from NancyLi1013 October 9, 2020 15:02
@NancyLi1013
Copy link
Contributor

Hi @RDCH106
I checked the build results here https://dev.azure.com/vcpkg/public/_build/results?buildId=43808&view=logs&j=d3670d0c-cc82-50c9-762c-eccde036829f and found that:

The failures on Linux and osx are due to this:

c++: error: /bigobj: No such file or directory

Since /bigobj is only used in MSVC.

The failures on uwp are due to this:

CMake Error at ports/rbdl/portfile.cmake:29 (file):
  file COPY cannot find "D:/buildtrees/rbdl/x64-uwp-dbg/rbdl.dll": No error.
Call Stack (most recent call first):
  scripts/ports.cmake:79 (include)

Generally, CMake will install the targets to the related directory automatically and we don't need to handle it manually.
So this part might not be needed.

if(VCPKG_LIBRARY_LINKAGE STREQUAL dynamic)
    if(VCPKG_TARGET_IS_WINDOWS)
        if(NOT DEFINED VCPKG_BUILD_TYPE OR VCPKG_BUILD_TYPE STREQUAL "debug")
            file(COPY ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-dbg/rbdl.dll DESTINATION ${CURRENT_PACKAGES_DIR}/debug/bin)
        endif()
        if(NOT DEFINED VCPKG_BUILD_TYPE OR VCPKG_BUILD_TYPE STREQUAL "release")
            file(COPY ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel/rbdl.dll DESTINATION ${CURRENT_PACKAGES_DIR}/bin)
        endif()
    endif()
endif()

You can make a patch to add
RUNTIME DESTINATION ${CMAKE_INSTALL_LIBDIR} to install targets in dynamic build.

Based on the above situation, I think this port supports all platform. So we can remove Supports field now and try to fix the failures and rebuild this.

Note: You can merge the master branch to this PR when you do the changes.

@RDCH106
Copy link
Contributor Author

RDCH106 commented Oct 10, 2020

Just one more question to be more self-reliant next time. The error due /bigobj only supported by MSVC is not showed here: https://dev.azure.com/vcpkg/public/_build/results?buildId=43808&view=logs&j=d3670d0c-cc82-50c9-762c-eccde036829f

imagen

But how can I inspect /mnt/vcpkg-ci/buildtrees/rbdl/install-x64-linux-dbg-out.log? Is it any way to access the log produced by the CI to see that file?

Thanks in advance and sorry for my many questions

RDCH106 and others added 2 commits October 10, 2020 09:49
…patch.diff

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
RDCH106 and others added 3 commits October 10, 2020 09:58
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
@RDCH106 RDCH106 requested a review from NancyLi1013 October 10, 2020 08:19
@RDCH106
Copy link
Contributor Author

RDCH106 commented Oct 10, 2020

@NancyLi1013 removing Supports field and vcpkg_fail_port_install function produces global failure. It have no sense for me...

@RDCH106
Copy link
Contributor Author

RDCH106 commented Oct 10, 2020

I begin to understand... The successfully builds are because they are skipping CMake building 😅, but I don't know how to resolve it because have no idea how to see CMake logs in Azure DevOps.

@NancyLi1013
Copy link
Contributor

Hi @RDCH106

The failures are caused by the patch. It seems you didn't update the patch. So It failed to apply.

CMake Error at scripts/cmake/vcpkg_apply_patches.cmake:54 (message):
  Applying patch failed.  error: corrupt patch at line 16

Call Stack (most recent call first):
  scripts/cmake/vcpkg_extract_source_archive_ex.cmake:141 (vcpkg_apply_patches)
  scripts/cmake/vcpkg_from_github.cmake:139 (vcpkg_extract_source_archive_ex)
  ports/rbdl/portfile.cmake:7 (vcpkg_from_github)
  scripts/ports.cmake:79 (include)

You need to make a new patch.

@NancyLi1013
Copy link
Contributor

@RDCH106
Copy link
Contributor Author

RDCH106 commented Oct 10, 2020

I think it's ready 😁

@NancyLi1013, thank you again for your huge job and patient with me.

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Oct 12, 2020
@NancyLi1013
Copy link
Contributor

Thanks for your efforts to this PR. @RDCH106
LGTM now.

@dan-shaw dan-shaw merged commit 30d39c8 into microsoft:master Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants