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

[sdformat6] Migrate from Bitbucket to GitHub 🤖 #10859

Merged

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Apr 16, 2020

As announced in https://community.gazebosim.org/t/important-gazebo-and-ignition-are-going-to-github/533, the sdformat repository has been migrated from Bitbucket to GitHub.

This commit also updates the hash as apparently the archive generated by GitHub is slightly different from the one generated by Bitbucket.

This PR is similar to #10858 .

Describe the pull request

As announced in https://community.gazebosim.org/t/important-gazebo-and-ignition-are-going-to-github/533,
the sdformat repository has been migrated from Bitbucket to GitHub.

This commit also updates the hash as apparently the archive generated by GitHub is slightly different
from the one generated by Bitbucket.
@traversaro traversaro changed the title [sdformat6] Migrate from Bitbucket to GitHub [sdformat6] Migrate from Bitbucket to GitHub 🤖 Apr 16, 2020
@NancyLi1013 NancyLi1013 self-assigned this Apr 17, 2020
@@ -1,10 +1,10 @@
include(vcpkg_common_functions)
Copy link
Contributor

Choose a reason for hiding this comment

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

include(vcpkg_common_functions) is no long needed.
Could you please remove this line?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also add vcpkg_fail_port_install(ON_ARCH "arm" ON_TARGET "uwp") here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, no problem. However typically it is common practice in open source project to prefer self-contained PRs that just fix a single problem, and don't mix solving the issue with other (even if required/important) changes. If vcpkg instead suggest to do all the deprecation-related cleanup whenever a port is modified in a PR, that is perfect ok but I suggest to describe this policy in the Mantainer Guideline: https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#avoid-trivial-changes-in-untouched-files .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c6ad5af .

OUT_SOURCE_PATH SOURCE_PATH
REPO osrf/sdformat
REF sdformat6_6.2.0
SHA512 3e3934010438bffbf10c1df29bd486c098e3c1bdf2b0349b69a53fb6f4d2bd3b3c8c4b4a8dfb413da13a638c0794f41c1bff4adb11a889b1552d90ba8b94c495
SHA512 3d139ec4b4c9fbfd547ed8bfca0adb5cdca92c1b7cc4d4b554a7c51ccf755b9079c26a006ebfedc5bc5b1ba5e16ad950bb38c47ea97bf97e59a2fd7d12d60620
HEAD_REF sdf6
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please update the way to handle to copyright as
file(INSTALL ${SOURCE_PATH}/LICENSE DESTINATION ${CURRENT_PACKAGES_DIR}/share/${PORT} RENAME copyright)?

Copy link
Contributor

Choose a reason for hiding this comment

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

vcpkg_test_cmake() is deprecated.
Could you please remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c6ad5af .

@@ -1,5 +1,5 @@
Source: sdformat6
Version: 6.2.0
Version: 6.2.0-1
Homepage: http://sdformat.org/
Build-Depends: boost-any, boost-variant, ignition-math4, urdfdom, tinyxml
Description: Simulation Description Format (SDF) parser and description files.
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that sdformat6 seemed to not support arm and uwp.
So could you please add Supports: !(arm|uwp) field to CONTROL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c6ad5af .

@NancyLi1013
Copy link
Contributor

Hi @traversaro
Thanks for this PR.
I noticed the regressions on Windows platform like this:

Starting package 31/31: sdformat6:x64-windows
Building package sdformat6[core]:x64-windows...
Could not locate cached archive: C:\vsts\_work\4\s\archives\3d\3d924945b47fca0a9318848ebcf70b1cb1564b5f.zip
-- Using cached C:/vsts/_work/4/s/downloads/osrf-sdformat-sdformat6_6.2.0.tar.gz
CMake Error at scripts/cmake/vcpkg_download_distfile.cmake:99 (message):
  

  File does not have expected hash:

          File path: [ C:/vsts/_work/4/s/downloads/osrf-sdformat-sdformat6_6.2.0.tar.gz ]
      Expected hash: [ 3d139ec4b4c9fbfd547ed8bfca0adb5cdca92c1b7cc4d4b554a7c51ccf755b9079c26a006ebfedc5bc5b1ba5e16ad950bb38c47ea97bf97e59a2fd7d12d60620 ]
        Actual hash: [ 3e3934010438bffbf10c1df29bd486c098e3c1bdf2b0349b69a53fb6f4d2bd3b3c8c4b4a8dfb413da13a638c0794f41c1bff4adb11a889b1552d90ba8b94c495 ]

  Please delete the file and retry if this file should be downloaded again.

It seems that the hash value should be kept as the before one.
Could you please check it again?

@traversaro
Copy link
Contributor Author

Could you please check it again?

I think there is an unfortunate problem: as vcpkg_from_bitbucket and vcpkg_from_github create different archives but with the exact same names, the CI (and also existing installation) thinks that it is ok to use the bitbucket-downloaded file. But if you install the port in a clean vcpkg installation, the required hash is the one that I updated. See #10858 (comment) for a related comment.

@traversaro
Copy link
Contributor Author

All the cleanup should have been addressed in c6ad5af . .

@traversaro
Copy link
Contributor Author

Could you please check it again?

I think there is an unfortunate problem: as vcpkg_from_bitbucket and vcpkg_from_github create different archives but with the exact same names, the CI (and also existing installation) thinks that it is ok to use the bitbucket-downloaded file. But if you install the port in a clean vcpkg installation, the required hash is the one that I updated. See #10858 (comment) for a related comment.

If it ok for you, I can avoid that by modifying vcpkg_from_github to accept an optional argument that specify a suffix for the downloaded archive name, so that we can avoid this unfortunate naming collision.

@traversaro
Copy link
Contributor Author

If it ok for you, I can avoid that by modifying vcpkg_from_github to accept an optional argument that specify a suffix for the downloaded archive name, so that we can avoid this unfortunate naming collision.

An alternative is to modify the names of the archives created by vcpkg_from_github to contain a reference to github and avoid this kind of problems in the future, but I guess that would invalidate the downloads cache in CI for a a lot of ports.

ports/sdformat6/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed waiting for response labels Apr 20, 2020
@ras0219-msft ras0219-msft merged commit 7873205 into microsoft:master Apr 21, 2020
@ras0219-msft
Copy link
Contributor

Thanks for the PR!

An alternative is to modify the names of the archives created by vcpkg_from_github to contain a reference to github and avoid this kind of problems in the future, but I guess that would invalidate the downloads cache in CI for a a lot of ports.

This seems very reasonable to me, but instead modifying vcpkg_from_bitbucket().

@traversaro traversaro deleted the sdformat-bitbucket-github-migration branch April 22, 2020 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants