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.

Sign up for GitHub

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

Jump to bottom

ignition-msgs5: Add support for cross-compilation #11538

Closed
wants to merge 5 commits into from
Closed

ignition-msgs5: Add support for cross-compilation #11538

wants to merge 5 commits into from

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented May 23, 2020 •
edited

  • What does your PR fix?
    This PR enables compilation of the ignition-msgs5 port on triplet that require compilation, i.e. among official supported triplets arm64-windows and arm-uwp .

  • Which triplets are supported/not supported? Have you updated the CI baseline?
    This PR add support for compiling ignition-msgs5 for arm64-windows and arm-uwp, but I did not updated the CI baseline as the port will fail if ignition-msgs5:x86-windows is not installed, and I do not know how this is supposed to be handled at the CI baseline level.

  • Does your PR follow the maintainer guide?
    Yes.

This PR also required some changes in the protobuf ( fd8143b ) and ignition-modularscripts (5405eda) ports, feel free to check the commit messages for more details on those changes.

@traversaro traversaro mentioned this pull request May 23, 2020
Merged
@traversaro traversaro force-pushed the ignition-msgs-support-crosscompilation branch 4 times, most recently from a48e208 to c8bf0b0 Compare May 24, 2020 08:50
@NancyLi1013 NancyLi1013 self-assigned this May 25, 2020
NancyLi1013
NancyLi1013 suggested changes May 25, 2020
View reviewed changes
ports/ignition-modularscripts/CONTROL Outdated Show resolved Hide resolved
ports/ignition-msgs5/CONTROL Show resolved Hide resolved
@NancyLi1013 NancyLi1013 added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist requires:author-response labels May 25, 2020
@NancyLi1013 NancyLi1013 mentioned this pull request May 26, 2020
Merged
@NancyLi1013 NancyLi1013 added requires:testing Needs tests added before merging and removed requires:author-response labels May 26, 2020
@NancyLi1013
Copy link
Contributor

I will test the features later. Once finished, I will update the results.

@NancyLi1013
Copy link
Contributor

The feature has passed with the following triplets:

  • x86-windows
  • x64-windows
  • x64-windows-static

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:testing Needs tests added before merging labels May 27, 2020
@dan-shaw
Copy link
Contributor

dan-shaw commented Jun 5, 2020

@traversaro I just merged a protobuf PR, could you resolve the conflicts?

@traversaro traversaro force-pushed the ignition-msgs-support-crosscompilation branch from 4a9d4b3 to b155d09 Compare June 6, 2020 07:01
@traversaro
Copy link
Contributor Author

@traversaro I just merged a protobuf PR, could you resolve the conflicts?

Done.

@traversaro
Copy link
Contributor Author

x86_windows and x64_osx faiures seems to be unrelated, while the arm64-windows fails with the following message:

CMake Error at ports/ignition-msgs5/portfile.cmake:29 (message):
  Cross-targetting ignition-msgs5 requires the x86-windows ignition-msgs5 to
  be available.  Please install ignition-msgs5:x86-windows first.
Call Stack (most recent call first):
  scripts/ports.cmake:76 (include)

That from #11538 (comment) it should be something that instead should be installed in the CI machines.

@NancyLi1013
Copy link
Contributor

@traversaro
Could you please resolve the conflicts?

@traversaro traversaro force-pushed the ignition-msgs-support-crosscompilation branch from 8ecea64 to 3ee8fbc Compare June 11, 2020 14:22
@traversaro
Copy link
Contributor Author

@traversaro
Could you please resolve the conflicts?

Done.

@traversaro
Copy link
Contributor Author

The arm64-windows failure is due to:

CMake Error at ports/ignition-msgs5/portfile.cmake:29 (message):
Cross-targetting ignition-msgs5 requires the x86-windows ignition-msgs5 to
be available. Please install ignition-msgs5:x86-windows first.
Call Stack (most recent call first):
scripts/ports.cmake:76 (include)

Based on #11538 (comment) , this should be something installed in the CI machines.

@NancyLi1013
Copy link
Contributor

@traversaro
The regression on x64-osx should be fixed in this PR #11616.

CMake Error in src/CMakeLists.txt:
  Imported target "TINYXML2::TINYXML2" includes non-existent path

    "/Volumes/data/work/1/s/packages/tinyxml2_x64-osx/debug/include"

  in its INTERFACE_INCLUDE_DIRECTORIES.  Possible reasons include:

  * The path was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and references files it does not
  provide.

Could you please merge the latest commit on master to this PR?

@traversaro traversaro force-pushed the ignition-msgs-support-crosscompilation branch from 3ee8fbc to ca2defd Compare June 15, 2020 21:33
@traversaro
Copy link
Contributor Author

Could you please merge the latest commit on master to this PR?

Done.

@NancyLi1013
Copy link
Contributor

Seems it doesn't work. I noticed that this was the same reason with #11616. Could you look into this again?

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013
Copy link
Contributor

It seems the command /azp run doesn't work.

@traversaro
Could you please merge master to retrigger this PR?

@traversaro traversaro force-pushed the ignition-msgs-support-crosscompilation branch from ab802c9 to b2c8e94 Compare July 18, 2020 13:56
@traversaro
Copy link
Contributor Author

It seems the command /azp run doesn't work.

@traversaro
Could you please merge master to retrigger this PR?

I rebased the PR on the top of latest master.

JackBoosY
JackBoosY suggested changes Jul 20, 2020
View reviewed changes
ports/protobuf/CONTROL Show resolved Hide resolved
@traversaro
Copy link
Contributor Author

Sorry @JackBoosY, I had missed you request change. However, I solved the conflicts w.r.t. to the latest master.

@JackBoosY
Copy link
Contributor

Sorry for late, I lost track with this PR.
These changes caused protobuf build failure on uwp triplet, could you fix it?

     6>D:\buildtrees\protobuf\src\dfb7589f92-61fbc4fa5f.clean\src\google\protobuf\compiler\subprocess.cc(104,46): error C2065: 'HANDLE_FLAG_INHERIT': undeclared identifier [D:\buildtrees\protobuf\arm-uwp-dbg\libprotoc.vcxproj]
     6>D:\buildtrees\protobuf\src\dfb7589f92-61fbc4fa5f.clean\src\google\protobuf\compiler\subprocess.cc(105,29): error C2065: 'HANDLE_FLAG_INHERIT': undeclared identifier [D:\buildtrees\protobuf\arm-uwp-dbg\libprotoc.vcxproj]
     6>D:\buildtrees\protobuf\src\dfb7589f92-61fbc4fa5f.clean\src\google\protobuf\compiler\subprocess.cc(109,48): error C2065: 'HANDLE_FLAG_INHERIT': undeclared identifier [D:\buildtrees\protobuf\arm-uwp-dbg\libprotoc.vcxproj]
     6>D:\buildtrees\protobuf\src\dfb7589f92-61fbc4fa5f.clean\src\google\protobuf\compiler\subprocess.cc(110,29): error C2065: 'HANDLE_FLAG_INHERIT': undeclared identifier [D:\buildtrees\protobuf\arm-uwp-dbg\libprotoc.vcxproj]
     6>D:\buildtrees\protobuf\src\dfb7589f92-61fbc4fa5f.clean\src\google\protobuf\compiler\subprocess.cc(119,26): error C2065: 'STARTF_USESTDHANDLES': undeclared identifier [D:\buildtrees\protobuf\arm-uwp-dbg\libprotoc.vcxproj]

@JackBoosY JackBoosY self-assigned this Aug 3, 2020
@NancyLi1013
Copy link
Contributor

@traversaro
Could you please also resolve the conflicts?

@traversaro
Copy link
Contributor Author

Hi @JackBoosY and @NancyLi1013 , I was away from keyboard for a bit, sorry for the lack of feedback. I will update the PR in the coming days, thanks!

@traversaro
protobuf: Do not install malformed CMake configuration files when cro…
c24aa1a 
…sscompiling

Before this commit, the protobuf portfile when crosscompiling was not defining the
protobuf::protoc target in its CMake config files, even if its definition was instead
correctly patched in the port to point to the host version of the tool. Furthermore,
the protobuf::libprotoc library (that is a library that can also be used in
cross-compilation build) was not installed.
@traversaro
ignition-modularscripts: Add options to handle tools and custom CMAke…
cfd98a6 
… options
@traversaro
ignition-msgs5: Add support for crosscompilation
6921ebe 
Similar to the protobuf case, when cross-compiling it is required
that the port is already installed in the host machine triplet,
for now just x86-windows as for protobuf.
@traversaro traversaro force-pushed the ignition-msgs-support-crosscompilation branch 2 times, most recently from 269e79a to 6921ebe Compare September 5, 2020 09:16
@traversaro
Copy link
Contributor Author

@traversaro
Could you please also resolve the conflicts?

Hi @NancyLi1013 , the conflicts have been solved. @JackBoosY let's see if the uwp failure are still there, probably there is some upstream protobuf regression regarding compilation of the libprotoc library on uwp.

@traversaro
Copy link
Contributor Author

There are several failures, but they do not seems to be related to this PR.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal
Copy link
Member

The failures in CI are consistent with a malformed CONTROL file somewhere and they aren't happening outside of this PR. Just in case I pushed a merge with master, we'll see what comes back.

@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@JackBoosY
Copy link
Contributor

@BillyONeal Any progress?

@traversaro
Copy link
Contributor Author

@BillyONeal Any progress?

The failure on this are due to a regression in protobuf, that does not support anymore in the latest version compilation in uwp .
As they are already some active PR in the direction of getting protobuf to correctly cross compile (see #14165), let's try to see if those PR before starting to work again on this one.

@traversaro traversaro closed this Oct 31, 2020
@traversaro traversaro mentioned this pull request Nov 4, 2020
Merged
@traversaro
Copy link
Contributor Author

Related PR: #15424 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JackBoosY JackBoosY JackBoosY requested changes

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

@dan-shaw dan-shaw dan-shaw approved these changes

Assignees

@NancyLi1013 NancyLi1013

@JackBoosY JackBoosY

Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist depends:different-pr This PR or Issue depends on a PR which has been filed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@traversaro @NancyLi1013 @dan-shaw @orudge @JackBoosY @BillyONeal