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

[cpuinfo,fbgemm,nnpack] update to latest source version and rename targets #17063

Merged
merged 13 commits into from
May 5, 2021
Merged

[cpuinfo,fbgemm,nnpack] update to latest source version and rename targets #17063

merged 13 commits into from
May 5, 2021

Conversation

luncliff
Copy link
Contributor

@luncliff luncliff commented Apr 3, 2021

What does your PR fix?

The previous patch cpuinfo-pull-22-868bd11.patch, which is from pytorch/cpuinfo#22 failed to apply.

Starting package 1/1: cpuinfo:x64-osx
Building package cpuinfo[core]:x64-osx...
-- Using cached /Users/user/vcpkg/downloads/cpuinfo-pull-22-868bd11.patch
-- Using cached /Users/user/vcpkg/downloads/pytorch-cpuinfo-5916273f79a21551890fd3d56fc5375a78d1598d.tar.gz
-- Extracting source /Users/user/vcpkg/downloads/pytorch-cpuinfo-5916273f79a21551890fd3d56fc5375a78d1598d.tar.gz
-- Applying patch /Users/user/vcpkg/downloads/cpuinfo-pull-22-868bd11.patch
CMake Error at scripts/cmake/z_vcpkg_apply_patches.cmake:57 (message):
  Applying patch failed: Checking patch CMakeLists.txt...

  error: while searching for:

  OPTION(CPUINFO_BUILD_MOCK_TESTS "Build cpuinfo mock tests" ON)

  OPTION(CPUINFO_BUILD_BENCHMARKS "Build cpuinfo micro-benchmarks" ON)

  

  # ---[ CMake options

  IF(CPUINFO_BUILD_UNIT_TESTS OR CPUINFO_BUILD_MOCK_TESTS)

    ENABLE_TESTING()

  

  error: patch failed: CMakeLists.txt:17

  error: CMakeLists.txt: patch does not apply

  Checking patch cmake/Config.cmake.in...

  Checking patch deps/clog/CMakeLists.txt...

Call Stack (most recent call first):
  scripts/cmake/vcpkg_extract_source_archive_ex.cmake:144 (z_vcpkg_apply_patches)
  scripts/cmake/vcpkg_from_github.cmake:154 (vcpkg_extract_source_archive_ex)
  ports/cpuinfo/portfile.cmake:11 (vcpkg_from_github)
  scripts/ports.cmake:142 (include)

Picked those differences and created a new patch file.
What a sad that PR is not merged yet ...

Which triplets are supported/not supported? Have you updated the CI baseline?

No change in triplet support. The current build check must be passed.

Target namespace changed

The package cpuinfo:... provides CMake targets:

    find_package(unofficial-cpuinfo CONFIG REQUIRED)
    target_link_libraries(main PRIVATE unofficial::cpuinfo::clog unofficial::cpuinfo::cpuinfo)

Does your PR follow the maintainer guide?

I think so.

@luncliff luncliff changed the title [cpuinfo] update port to latest [cpuinfo] update to latest source version Apr 3, 2021
@JackBoosY JackBoosY self-assigned this Apr 5, 2021
@JackBoosY JackBoosY added the category:port-update The issue is with a library, which is requesting update new revision label Apr 5, 2021
Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

Since this is no the upstream changes, we should add prefix unofficial-.

@luncliff
Copy link
Contributor Author

luncliff commented Apr 7, 2021

Cool. There are some ports I worked to use this, but let me fix them after this

@JackBoosY
Copy link
Contributor

I think you should remake the patch file.

* fixup target path to `share/unofficial-cpuinfo`
* change IMPORTED target name to `unofficial::cpuinfo::cpuinfo`
* change cpuinfo::cpuinfo to unofficial
@luncliff luncliff changed the title [cpuinfo] update to latest source version [cpuinfo,fbgemm] update to latest source version and rename targets Apr 10, 2021
@luncliff
Copy link
Contributor Author

luncliff commented Apr 10, 2021

bcb6171: Merged master to resolve possible conflicts with #16344

@luncliff luncliff changed the title [cpuinfo,fbgemm] update to latest source version and rename targets [cpuinfo,fbgemm,nnpack] update to latest source version and rename targets Apr 10, 2021
@luncliff luncliff mentioned this pull request Apr 10, 2021
19 tasks
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Apr 11, 2021
@strega-nil-ms
Copy link
Contributor

I think that that's incorrect; they're just corrections of upstream targets, so they should be under the upstream name, not unofficial-; @JackBoosY do you disagree?

@JackBoosY
Copy link
Contributor

@strega-nil-ms The upstream doesn't export any targets in the master branch, did I miss something?

@strega-nil-ms
Copy link
Contributor

@JackBoosY oh... yeah you're right, I totally misread that, you're right.

@strega-nil-ms strega-nil-ms merged commit 4ef97c2 into microsoft:master May 5, 2021
@strega-nil-ms
Copy link
Contributor

Thanks @luncliff :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants