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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ignition] Updates on various pkgs 馃 #14324

Merged
merged 16 commits into from Nov 6, 2020
Merged

[ignition] Updates on various pkgs 馃 #14324

merged 16 commits into from Nov 6, 2020

Conversation

ahoarau
Copy link
Contributor

@ahoarau ahoarau commented Oct 31, 2020

This PR updates various ignition-robotics ports (according to the dome release https://ignitionrobotics.org/docs/dome/install) :

Updates

  • ignition-cmake2 2.2.0-1 --> 2.5.0
  • ignition-math6 6.4.0 --> 6.6.0
  • ignition-msgs5 5.1.0 --> 5.3.0
  • ignition-transport8 8.0.0 --> 8.1.0

New packages:

  • ignition-msgs6 6.0.0
  • ignition-transport9 9.0.0
  • sdformat10 10.0.0

@ghost
Copy link

ghost commented Oct 31, 2020

CLA assistant check
All CLA requirements met.

@traversaro
Copy link
Contributor

traversaro commented Oct 31, 2020

Cool, I think that there is a partial overlap with #14323 and #11273, but probably it is better to get this PR to work instead. One comment: I think that sdformat10 depends on tinyxml2, not tinyxml.

@traversaro
Copy link
Contributor

After fixing the .pc files installed by console-bridge, urdfdom-headers and urdfdom, and after updating the reason why the ignition-transport library can't install their .pc files (it used to be #11817, now it is #14327), I think this PR is ready for review.

@ahoarau
Copy link
Contributor Author

ahoarau commented Oct 31, 2020

Well done ! Thanks for the help Silvio

@JackBoosY JackBoosY added the category:port-update The issue is with a library, which is requesting update new revision label Nov 2, 2020
ports/ignition-msgs6/CONTROL Outdated Show resolved Hide resolved
@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Nov 3, 2020
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Nov 3, 2020
Comment on lines +10 to +12
vcpkg_add_to_path(${CURRENT_INSTALLED_DIR}/bin)
vcpkg_add_to_path(${CURRENT_INSTALLED_DIR}/debug/bin)

Copy link
Member

Choose a reason for hiding this comment

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

FYI, these are why the ignition things are almost always failing in our CI builds: if the llvm port is installed, then there's a clang.exe here, and cmake thinks you're targeting clang even though that clang.exe is not entirely functional :(

No change requested directly but something you might want to be thinking about. We are investigating fixing the llvm port to not put clang.exe here (although protobuf should not be putting any exes here either)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @BillyONeal, sorry for that, I was not aware of the problem at all, sorry for causing troubles. As the dll need to be found in the path only at the build phase, probably the simplest fix is to put the modification of the path after the configure and before the build, i.e. in

. This could activated with a dedicated option to the ignition_modular_library, or if we don't think there could be downside, we can just do that for all ignition ports, to follow KISS.

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, note that the workaround of adding those directories to the path is not necessary because protobuf installs some .exe in the ${CURRENT_INSTALLED_DIR}/bin or ${CURRENT_INSTALLED_DIR}/debug/bin . The problem is that ignition-msgs is creating its own protobuf-based code generator using the protoc library provided by protobuf, that is called ign_msgs_gen. As the build executes ign_msgs_gen to generate the code that is necessary to compile as part of the build, if the port is compiled with a shared library triplet such as x64-windows, it needs to be able to find the necessary protobuf.dll and protoc.dll libraries. A more general fix would be to copy the dependent .dll for executables in the build directory as we do for the executables in the build directory as we do for the one in the install directory, or at least just copy the necessary .dll manually in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

A more general fix would be to copy the dependent .dll for executables in the build directory as we do for the executables in the build directory as we do for the one in the install directory

Actually, I checked in https://github.com/microsoft/vcpkg/blob/master/scripts/buildsystems/vcpkg.cmake and it seems that it should be already the case, so perhaps it could be sufficient to try to drop those modification of the PATH.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @BillyONeal, sorry for that, I was not aware of the problem at all, sorry for causing troubles.

Oh, nothing to be sorry about! I just wanted to let you know since you seem to be a contributor who cares about these ports!

@BillyONeal BillyONeal merged commit 7fbbc59 into microsoft:master Nov 6, 2020
Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Nov 6, 2020
[ignition] Updates on various pkgs 馃  (microsoft#14324)
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! category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants