Skip to content

Fix cmake_minimum_require in libshm #58306

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
Closed
wants to merge 1 commit into from
Closed

Fix cmake_minimum_require in libshm #58306

wants to merge 1 commit into from

Conversation

zhouzhuojie
Copy link
Contributor

Deprecation warning reported by cmake:

CMake Deprecation Warning at CMakeLists.txt (cmake_minimum_required):
  Compatibility with CMake < 2.8.12 will be removed from a future version of CMake.
  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.

This is the only place that requires bumping min version. There're two others but only in third_party folder.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 14, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit a20474d (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@zhouzhuojie zhouzhuojie requested a review from a team May 14, 2021 17:43
janeyx99
janeyx99 approved these changes May 14, 2021
View reviewed changes
Copy link
Contributor

@janeyx99 janeyx99 left a comment

Choose a reason for hiding this comment

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

looks fine to me

@facebook-github-bot
Copy link
Contributor

@zhouzhuojie has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

walterddr
walterddr reviewed May 14, 2021
View reviewed changes
torch/lib/libshm/CMakeLists.txt
@@ -1,6 +1,6 @@
project(libshm C CXX)
cmake_minimum_required(VERSION 2.6 FATAL_ERROR)
cmake_policy(VERSION 2.6)
cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR)
Copy link
Contributor

Choose a reason for hiding this comment

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

I dunno why we dont just bump to the minimum requirement of PyTorch main project. which is already 3.5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't know for sure, but it's possible people can import pytorch's internal as libraries, and they may maintain their own cmake version. keeping the lowest viable version gives the max compatibility I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update to 3.5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, "shipit" was in queue before the comment, do you think we can bump up the min version for all cmake_minimum_required within pytorch repo? I can followup with a PR to change it

@zhouzhuojie zhouzhuojie force-pushed the fix_cmake_minimum_required_in_libshm branch from 2ba58f1 to a20474d Compare May 14, 2021 22:01
@codecov
Copy link

codecov bot commented May 15, 2021

Codecov Report

Merging #58306 (a20474d) into master (84d8e3b) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #58306   +/-   ##
=======================================
  Coverage   76.47%   76.47%           
=======================================
  Files        1992     1992           
  Lines      199897   199897           
=======================================
+ Hits       152866   152872    +6     
+ Misses      47031    47025    -6

malfet
malfet approved these changes May 17, 2021
View reviewed changes
torch/lib/libshm/CMakeLists.txt
@@ -1,6 +1,6 @@
project(libshm C CXX)
cmake_minimum_required(VERSION 2.6 FATAL_ERROR)
cmake_policy(VERSION 2.6)
cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR)
cmake_minimum_required(VERSION 3.5 FATAL_ERROR)

torch/lib/libshm/CMakeLists.txt
@@ -1,6 +1,6 @@
project(libshm C CXX)
cmake_minimum_required(VERSION 2.6 FATAL_ERROR)
cmake_policy(VERSION 2.6)
cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update to 3.5

@facebook-github-bot
Copy link
Contributor

@zhouzhuojie has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@zhouzhuojie merged this pull request in eab59ba.

krshrimali pushed a commit to krshrimali/pytorch that referenced this pull request May 19, 2021
@zhouzhuojie
Fix cmake_minimum_require in libshm (pytorch#58306)
4d068ca 
Summary:
Deprecation warning reported by cmake:

```
CMake Deprecation Warning at CMakeLists.txt (cmake_minimum_required):
  Compatibility with CMake < 2.8.12 will be removed from a future version of CMake.
  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.
```

This is the only place that requires bumping min version. There're two others but only in `third_party` folder.

Pull Request resolved: pytorch#58306

Reviewed By: bdhirsh

Differential Revision: D28446097

Pulled By: zhouzhuojie

fbshipit-source-id: af5ef50e61bd57dc36089ebe62db70ba0081864c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malfet malfet malfet approved these changes

@janeyx99 janeyx99 janeyx99 approved these changes

+2 more reviewers

@walterddr walterddr walterddr approved these changes

@samestep samestep samestep approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
cla signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zhouzhuojie @facebook-github-bot @malfet @walterddr @samestep @janeyx99