Skip to content

[ryu] Add new port#10167

Merged
strega-nil merged 7 commits intomicrosoft:masterfrom
kreuzerkrieg:ryu
Mar 5, 2020
Merged

[ryu] Add new port#10167
strega-nil merged 7 commits intomicrosoft:masterfrom
kreuzerkrieg:ryu

Conversation

@kreuzerkrieg
Copy link
Contributor

  • What does your PR fix?
    Add ryu port
  • Which triplets are supported/not supported?
    All except UWP
  • Have you updated the CI baseline?
    Yes
  • Does your PR follow the maintainer guide?
    Yes

@kreuzerkrieg
Copy link
Contributor Author

Could you please take a look what went wrong, the CI output does not give a clue

@kreuzerkrieg kreuzerkrieg mentioned this pull request Feb 21, 2020
@LilyWangL LilyWangL changed the title [ryu] [ryu] Add new port Feb 24, 2020
@JackBoosY
Copy link
Contributor

Please resolve these regressions, see windows linux osx log.

@kreuzerkrieg
Copy link
Contributor Author

Please resolve these regressions, see windows linux osx log.

Not sure I get it. Looks like there is no bazel on OSX. Is it ok? Then what the vcpkg_find_acquire_program(BAZEL) for?

As for Win/Lin.

-- Downloading https://github.com/ulfjack/ryu/archive/v2.0.tar.gz...
-- Extracting source C:/vsts/_work/4/s/downloads/ulfjack-ryu-v2.0.tar.gz
-- Using source at C:/vsts/_work/4/s/buildtrees/ryu/src/v2.0-3429a290df
CMake Error at scripts/cmake/vcpkg_execute_build_process.cmake:136 (message):
Command failed: bazel build //ryu
Working Directory: C:/vsts/_work/4/s/buildtrees/ryu/src/v2.0-3429a290df
See logs for more information:

Call Stack (most recent call first):
ports/ryu/portfile.cmake:12 (vcpkg_execute_build_process)
scripts/ports.cmake:90 (include)

Is there failed command out put somewhere?

@JackBoosY
Copy link
Contributor

@kreuzerkrieg I've added bazel in OSX.

@kreuzerkrieg
Copy link
Contributor Author

@kreuzerkrieg I've added bazel in OSX.

Thanks!
Any idea what is the meaning of Win/Lin error message?

@JackBoosY
Copy link
Contributor

@kreuzerkrieg

    Command failed: bazel build //ryu
    Working Directory: C:/vsts/_work/4/s/buildtrees/ryu/src/v2.0-3429a290df

Make sure the command is correct.

@dan-shaw
Copy link
Contributor

@kreuzerkrieg The tensorflow port has an example on how to use bazel in vcpkg. I remember you had to use /// instead of // in bazel build on Windows for the targets but I forgot why.

@kreuzerkrieg
Copy link
Contributor Author

@kreuzerkrieg The tensorflow port has an example on how to use bazel in vcpkg. I remember you had to use /// instead of // in bazel build on Windows for the targets but I forgot why.

AFAIR, tensorflow just checks if bazel installed, as for build it executes some python script. I will check the /// vs // once I get to Windows machine. However I dont get why it fails on Linux, looks like it is running fine on my Ubuntu machine

@kreuzerkrieg
Copy link
Contributor Author

Nailed it.
Took a clean machine to test the port. It failed the exact way it failed in Azure CI.
The reason the vcpkg_find_acquire_program(BAZEL) does not deliver. It does not install bazel on the target machine. Once installed manually the port completed successfully

@JackBoosY
Copy link
Contributor

@kreuzerkrieg You should use BAZEL downloaded by vcpkg, please see the example.

@JackBoosY JackBoosY added needs-further-review info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed waiting for response labels Mar 2, 2020
@dan-shaw
Copy link
Contributor

dan-shaw commented Mar 2, 2020

LGTM, failures seem unrelated

@dan-shaw
Copy link
Contributor

dan-shaw commented Mar 3, 2020

/azp run

@strega-nil
Copy link
Contributor

Alright, thanks @kreuzerkrieg ! This is a great addition :)

@strega-nil strega-nil merged commit 4b1444e into microsoft:master Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants