[Bugfix][CMake] Update the minimum CMake version to 3.18 - #12682

Merged
driazati merged 2 commits into
apache:mainfrom
slyubomirsky:cmakelist-min-version
Sep 29, 2022
Merged

[Bugfix][CMake] Update the minimum CMake version to 3.18#12682
driazati merged 2 commits into
apache:mainfrom
slyubomirsky:cmakelist-min-version

Conversation

@slyubomirsky

@slyubomirskyslyubomirsky commented Sep 2, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to @vinx13. However, the current CMake version check in CMakeLists.txt is not checking for a sufficiently high CMake version; this PR updates it.

Bug that prompted this: When building with CMake version 3.16 I had the following error: CUDA_STANDARD is set to invalid value '17'. Upgrading to the latest CMake (3.24) fixed it.

driazati
driazati approved these changes Sep 2, 2022
View reviewed changes

@driazatidriazati left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the cmake docs agree https://cmake.org/cmake/help/latest/prop_tgt/CUDA_STANDARD.html

Comment threadCMakeLists.txt
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.2)
cmake_minimum_required(VERSION 3.18)

@driazatidriazatiSep 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can you also update the requirements in install/from_source.rst to match?

@slyubomirskyslyubomirskySep 2, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Will do, thanks

junrushao
junrushao approved these changes Sep 2, 2022
View reviewed changes

@junrushaojunrushao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@slyubomirsky

slyubomirsky commented Sep 2, 2022

Copy link
Copy Markdown
ContributorAuthor

Interesting, it looks like the CI's CMake version is below 3.18. Is it not using C++17?

@junrushao

junrushao commented Sep 2, 2022

Copy link
Copy Markdown
Member

Likely nvcc is not used in the CI

@slyubomirsky

slyubomirsky commented Sep 2, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Should the version check be conditional, then? I am not especially familiar with CMake but I imagine it's probably possible.

I am a little confused as to how the CI passes with lower versions of CMake while I had a fresh setup that failed, unless it's not using C++17

@junrushao

junrushao commented Sep 2, 2022

Copy link
Copy Markdown
Member

I don't have much idea how to do such a check if we want to condition whether or not NVCC is used, but it seems possible if we want to warn globally no matter if nvcc is used

@areusch

areusch commented Sep 2, 2022

Copy link
Copy Markdown
Contributor

shall we also update https://github.com/apache/tvm/blob/main/docker/install/ubuntu_install_cmake_source.sh?

@driazatidriazati mentioned this pull request Sep 2, 2022
Merged
leandron
leandron reviewed Sep 2, 2022
View reviewed changes

@leandronleandron left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@slyubomirsky FYI, in #12131 I'm updating the version we build CMake from source to be compliant with what's being proposed here... 3.18.4 exactly. (cc @driazati)

@slyubomirsky

slyubomirsky commented Sep 2, 2022

Copy link
Copy Markdown
ContributorAuthor

Glad to hear the CI's CMake will be updated. I'll also update the install script.

@slyubomirsky

slyubomirsky commented Sep 2, 2022

Copy link
Copy Markdown
ContributorAuthor

Ah, I see the last change overlaps with @leandron's PR

yongwww
yongwww approved these changes Sep 2, 2022
View reviewed changes

@yongwwwyongwww left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@areusch

areusch commented Sep 7, 2022

Copy link
Copy Markdown
Contributor

looks like some issues with the CI, but retriggering as they may be unrelated.

@slyubomirsky

slyubomirsky commented Sep 7, 2022

Copy link
Copy Markdown
ContributorAuthor

Is the CI using a higher version of CMake? If not, I think the errors will continue 😅

@driazati

driazati commented Sep 7, 2022

Copy link
Copy Markdown
Member

#12131 just merged, we have to update the Docker images to include that before this PR can pass CI

@slyubomirsky

slyubomirsky commented Sep 9, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Per this PR, should I make the minimum version higher?

@slyubomirsky

slyubomirsky commented Sep 12, 2022

Copy link
Copy Markdown
ContributorAuthor

@driazati when do we expect there to be an update?

@driazati

driazati commented Sep 14, 2022

Copy link
Copy Markdown
Member

some of the images were updated to use 3.18 but not all of them, #12774 should clean up the rest so once that is merged and updated on the images this PR should work

@areusch

areusch commented Sep 15, 2022

Copy link
Copy Markdown
Contributor

@tvm-bot rerun

@slyubomirsky

slyubomirsky commented Sep 22, 2022

Copy link
Copy Markdown
ContributorAuthor

Hm, some of the builds still use different versions of CMake. Any pointers on where the CMake versions for those are defined? I would be glad to make the changes myself.

@driazati

driazati commented Sep 27, 2022

Copy link
Copy Markdown
Member

Sorry for the delay, #12906 included a Docker image tag update so they should all be on cmake 3.18+ plus now, a rebase on main should get this PR working

@slyubomirsky

slyubomirsky commented Sep 28, 2022

Copy link
Copy Markdown
ContributorAuthor

Thanks for the reply. Rebasing as advised

@driazati
driazati merged commit 8c88aab into apache:mainSep 29, 2022
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
@slyubomirsky@xinetzone
[Bugfix][CMake] Update the minimum CMake version to 3.18 (apache#12682)
47d5c44
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to vinx13. However, the current CMake version check in `CMakeLists.txt` is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: `CUDA_STANDARD is set to invalid value '17'`. Upgrading to the latest CMake (3.24) fixed it.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandronleandronleandron left review comments
@driazatidriazatidriazati approved these changes
@yongwwwyongwwwyongwww approved these changes
@junrushaojunrushaojunrushao approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@slyubomirsky@junrushao@areusch@driazati@leandron@yongwww
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
[Bugfix][CMake] Update the minimum CMake version to 3.18 - #12682
Merged
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Sep 29, 2022
Merged
[Bugfix][CMake] Update the minimum CMake version to 3.18#12682
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Conversation
@slyubomirsky
@slyubomirskyslyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to @vinx13. However, the current CMake version check in CMakeLists.txt is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: CUDA_STANDARD is set to invalid value '17'. Upgrading to the latest CMake (3.24) fixed it.
driazati
driazati
approved these changes
Sep 2, 2022
View reviewed changes
@driazatidriazati
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

the cmake docs agree https://cmake.org/cmake/help/latest/prop_tgt/CUDA_STANDARD.html
Comment threadCMakeLists.txt
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.2)
cmake_minimum_required(VERSION 3.18)
@driazatidriazatiSep 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

can you also update the requirements in install/from_source.rst to match?
@slyubomirskyslyubomirskySep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will do, thanks
junrushao
junrushao
approved these changes
Sep 2, 2022
View reviewed changes
@junrushaojunrushao
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Interesting, it looks like the CI's CMake version is below 3.18. Is it not using C++17?
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
Likely nvcc is not used in the CI
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Should the version check be conditional, then? I am not especially familiar with CMake but I imagine it's probably possible.
I am a little confused as to how the CI passes with lower versions of CMake while I had a fresh setup that failed, unless it's not using C++17
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
I don't have much idea how to do such a check if we want to condition whether or not NVCC is used, but it seems possible if we want to warn globally no matter if nvcc is used
@areusch
areusch
commented
Sep 2, 2022

Copy link


Copy Markdown

Contributor
shall we also update https://github.com/apache/tvm/blob/main/docker/install/ubuntu_install_cmake_source.sh?
@driazatidriazati
mentioned this pull request
Sep 2, 2022
 Merged

leandron
leandron
reviewed
Sep 2, 2022
View reviewed changes
@leandronleandron
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@slyubomirsky FYI, in #12131 I'm updating the version we build CMake from source to be compliant with what's being proposed here... 3.18.4 exactly. (cc @driazati)
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Glad to hear the CI's CMake will be updated. I'll also update the install script.
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Ah, I see the last change overlaps with @leandron's PR
yongwww
yongwww
approved these changes
Sep 2, 2022
View reviewed changes
@yongwwwyongwww
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

lgtm
@areusch
areusch
commented
Sep 7, 2022

Copy link


Copy Markdown

Contributor
looks like some issues with the CI, but retriggering as they may be unrelated.
@slyubomirsky
slyubomirsky
commented
Sep 7, 2022

Copy link


Copy Markdown

ContributorAuthor
Is the CI using a higher version of CMake? If not, I think the errors will continue 😅
@driazati
driazati
commented
Sep 7, 2022

Copy link


Copy Markdown

Member
#12131 just merged, we have to update the Docker images to include that before this PR can pass CI
@slyubomirsky
slyubomirsky
commented
Sep 9, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Per this PR, should I make the minimum version higher?
@slyubomirsky
slyubomirsky
commented
Sep 12, 2022

Copy link


Copy Markdown

ContributorAuthor
@driazati when do we expect there to be an update?
@driazati
driazati
commented
Sep 14, 2022

Copy link


Copy Markdown

Member
some of the images were updated to use 3.18 but not all of them, #12774 should clean up the rest so once that is merged and updated on the images this PR should work
@areusch
areusch
commented
Sep 15, 2022

Copy link


Copy Markdown

Contributor
@tvm-bot rerun
@slyubomirsky
slyubomirsky
commented
Sep 22, 2022

Copy link


Copy Markdown

ContributorAuthor
Hm, some of the builds still use different versions of CMake. Any pointers on where the CMake versions for those are defined? I would be glad to make the changes myself.
@driazati
driazati
commented
Sep 27, 2022

Copy link


Copy Markdown

Member
Sorry for the delay, #12906 included a Docker image tag update so they should all be on cmake 3.18+ plus now, a rebase on main should get this PR working
@slyubomirsky
slyubomirsky
commented
Sep 28, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks for the reply. Rebasing as advised
@driazati
driazati
merged commit 8c88aab
into
apache:mainSep 29, 2022
xinetzone pushed a commit
to daobook/tvm
that referenced
this pull request
Nov 25, 2022
@slyubomirsky@xinetzone
[Bugfix][CMake] Update the minimum CMake version to 3.18 (apache#12682)
47d5c44
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to vinx13. However, the current CMake version check in `CMakeLists.txt` is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: `CUDA_STANDARD is set to invalid value '17'`. Upgrading to the latest CMake (3.24) fixed it.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@leandronleandronleandron left review comments
@driazatidriazatidriazati approved these changes
@yongwwwyongwwwyongwww approved these changes
@junrushaojunrushaojunrushao approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@slyubomirsky@junrushao@areusch@driazati@leandron@yongwww

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[Bugfix][CMake] Update the minimum CMake version to 3.18 - #12682
Merged
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Sep 29, 2022
Merged
[Bugfix][CMake] Update the minimum CMake version to 3.18#12682
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Conversation
@slyubomirsky
@slyubomirskyslyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to @vinx13. However, the current CMake version check in CMakeLists.txt is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: CUDA_STANDARD is set to invalid value '17'. Upgrading to the latest CMake (3.24) fixed it.
driazati
driazati
approved these changes
Sep 2, 2022
View reviewed changes
@driazatidriazati
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

the cmake docs agree https://cmake.org/cmake/help/latest/prop_tgt/CUDA_STANDARD.html
Comment threadCMakeLists.txt
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.2)
cmake_minimum_required(VERSION 3.18)
@driazatidriazatiSep 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

can you also update the requirements in install/from_source.rst to match?
@slyubomirskyslyubomirskySep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will do, thanks
junrushao
junrushao
approved these changes
Sep 2, 2022
View reviewed changes
@junrushaojunrushao
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Interesting, it looks like the CI's CMake version is below 3.18. Is it not using C++17?
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
Likely nvcc is not used in the CI
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Should the version check be conditional, then? I am not especially familiar with CMake but I imagine it's probably possible.
I am a little confused as to how the CI passes with lower versions of CMake while I had a fresh setup that failed, unless it's not using C++17
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
I don't have much idea how to do such a check if we want to condition whether or not NVCC is used, but it seems possible if we want to warn globally no matter if nvcc is used
@areusch
areusch
commented
Sep 2, 2022

Copy link


Copy Markdown

Contributor
shall we also update https://github.com/apache/tvm/blob/main/docker/install/ubuntu_install_cmake_source.sh?
@driazatidriazati
mentioned this pull request
Sep 2, 2022
 Merged

leandron
leandron
reviewed
Sep 2, 2022
View reviewed changes
@leandronleandron
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@slyubomirsky FYI, in #12131 I'm updating the version we build CMake from source to be compliant with what's being proposed here... 3.18.4 exactly. (cc @driazati)
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Glad to hear the CI's CMake will be updated. I'll also update the install script.
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Ah, I see the last change overlaps with @leandron's PR
yongwww
yongwww
approved these changes
Sep 2, 2022
View reviewed changes
@yongwwwyongwww
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

lgtm
@areusch
areusch
commented
Sep 7, 2022

Copy link


Copy Markdown

Contributor
looks like some issues with the CI, but retriggering as they may be unrelated.
@slyubomirsky
slyubomirsky
commented
Sep 7, 2022

Copy link


Copy Markdown

ContributorAuthor
Is the CI using a higher version of CMake? If not, I think the errors will continue 😅
@driazati
driazati
commented
Sep 7, 2022

Copy link


Copy Markdown

Member
#12131 just merged, we have to update the Docker images to include that before this PR can pass CI
@slyubomirsky
slyubomirsky
commented
Sep 9, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Per this PR, should I make the minimum version higher?
@slyubomirsky
slyubomirsky
commented
Sep 12, 2022

Copy link


Copy Markdown

ContributorAuthor
@driazati when do we expect there to be an update?
@driazati
driazati
commented
Sep 14, 2022

Copy link


Copy Markdown

Member
some of the images were updated to use 3.18 but not all of them, #12774 should clean up the rest so once that is merged and updated on the images this PR should work
@areusch
areusch
commented
Sep 15, 2022

Copy link


Copy Markdown

Contributor
@tvm-bot rerun
@slyubomirsky
slyubomirsky
commented
Sep 22, 2022

Copy link


Copy Markdown

ContributorAuthor
Hm, some of the builds still use different versions of CMake. Any pointers on where the CMake versions for those are defined? I would be glad to make the changes myself.
@driazati
driazati
commented
Sep 27, 2022

Copy link


Copy Markdown

Member
Sorry for the delay, #12906 included a Docker image tag update so they should all be on cmake 3.18+ plus now, a rebase on main should get this PR working
@slyubomirsky
slyubomirsky
commented
Sep 28, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks for the reply. Rebasing as advised
@driazati
driazati
merged commit 8c88aab
into
apache:mainSep 29, 2022
xinetzone pushed a commit
to daobook/tvm
that referenced
this pull request
Nov 25, 2022
@slyubomirsky@xinetzone
[Bugfix][CMake] Update the minimum CMake version to 3.18 (apache#12682)
47d5c44
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to vinx13. However, the current CMake version check in `CMakeLists.txt` is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: `CUDA_STANDARD is set to invalid value '17'`. Upgrading to the latest CMake (3.24) fixed it.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@leandronleandronleandron left review comments
@driazatidriazatidriazati approved these changes
@yongwwwyongwwwyongwww approved these changes
@junrushaojunrushaojunrushao approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@slyubomirsky@junrushao@areusch@driazati@leandron@yongwww

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[Bugfix][CMake] Update the minimum CMake version to 3.18 - #12682
Merged
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Sep 29, 2022
Merged
[Bugfix][CMake] Update the minimum CMake version to 3.18#12682
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Conversation
@slyubomirsky
@slyubomirskyslyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to @vinx13. However, the current CMake version check in CMakeLists.txt is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: CUDA_STANDARD is set to invalid value '17'. Upgrading to the latest CMake (3.24) fixed it.
driazati
driazati
approved these changes
Sep 2, 2022
View reviewed changes
@driazatidriazati
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

the cmake docs agree https://cmake.org/cmake/help/latest/prop_tgt/CUDA_STANDARD.html
Comment threadCMakeLists.txt
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.2)
cmake_minimum_required(VERSION 3.18)
@driazatidriazatiSep 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

can you also update the requirements in install/from_source.rst to match?
@slyubomirskyslyubomirskySep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will do, thanks
junrushao
junrushao
approved these changes
Sep 2, 2022
View reviewed changes
@junrushaojunrushao
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Interesting, it looks like the CI's CMake version is below 3.18. Is it not using C++17?
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
Likely nvcc is not used in the CI
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Should the version check be conditional, then? I am not especially familiar with CMake but I imagine it's probably possible.
I am a little confused as to how the CI passes with lower versions of CMake while I had a fresh setup that failed, unless it's not using C++17
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
I don't have much idea how to do such a check if we want to condition whether or not NVCC is used, but it seems possible if we want to warn globally no matter if nvcc is used
@areusch
areusch
commented
Sep 2, 2022

Copy link


Copy Markdown

Contributor
shall we also update https://github.com/apache/tvm/blob/main/docker/install/ubuntu_install_cmake_source.sh?
@driazatidriazati
mentioned this pull request
Sep 2, 2022
 Merged

leandron
leandron
reviewed
Sep 2, 2022
View reviewed changes
@leandronleandron
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@slyubomirsky FYI, in #12131 I'm updating the version we build CMake from source to be compliant with what's being proposed here... 3.18.4 exactly. (cc @driazati)
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Glad to hear the CI's CMake will be updated. I'll also update the install script.
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Ah, I see the last change overlaps with @leandron's PR
yongwww
yongwww
approved these changes
Sep 2, 2022
View reviewed changes
@yongwwwyongwww
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

lgtm
@areusch
areusch
commented
Sep 7, 2022

Copy link


Copy Markdown

Contributor
looks like some issues with the CI, but retriggering as they may be unrelated.
@slyubomirsky
slyubomirsky
commented
Sep 7, 2022

Copy link


Copy Markdown

ContributorAuthor
Is the CI using a higher version of CMake? If not, I think the errors will continue 😅
@driazati
driazati
commented
Sep 7, 2022

Copy link


Copy Markdown

Member
#12131 just merged, we have to update the Docker images to include that before this PR can pass CI
@slyubomirsky
slyubomirsky
commented
Sep 9, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Per this PR, should I make the minimum version higher?
@slyubomirsky
slyubomirsky
commented
Sep 12, 2022

Copy link


Copy Markdown

ContributorAuthor
@driazati when do we expect there to be an update?
@driazati
driazati
commented
Sep 14, 2022

Copy link


Copy Markdown

Member
some of the images were updated to use 3.18 but not all of them, #12774 should clean up the rest so once that is merged and updated on the images this PR should work
@areusch
areusch
commented
Sep 15, 2022

Copy link


Copy Markdown

Contributor
@tvm-bot rerun
@slyubomirsky
slyubomirsky
commented
Sep 22, 2022

Copy link


Copy Markdown

ContributorAuthor
Hm, some of the builds still use different versions of CMake. Any pointers on where the CMake versions for those are defined? I would be glad to make the changes myself.
@driazati
driazati
commented
Sep 27, 2022

Copy link


Copy Markdown

Member
Sorry for the delay, #12906 included a Docker image tag update so they should all be on cmake 3.18+ plus now, a rebase on main should get this PR working
@slyubomirsky
slyubomirsky
commented
Sep 28, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks for the reply. Rebasing as advised
@driazati
driazati
merged commit 8c88aab
into
apache:mainSep 29, 2022
xinetzone pushed a commit
to daobook/tvm
that referenced
this pull request
Nov 25, 2022
@slyubomirsky@xinetzone
[Bugfix][CMake] Update the minimum CMake version to 3.18 (apache#12682)
47d5c44
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to vinx13. However, the current CMake version check in `CMakeLists.txt` is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: `CUDA_STANDARD is set to invalid value '17'`. Upgrading to the latest CMake (3.24) fixed it.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@leandronleandronleandron left review comments
@driazatidriazatidriazati approved these changes
@yongwwwyongwwwyongwww approved these changes
@junrushaojunrushaojunrushao approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@slyubomirsky@junrushao@areusch@driazati@leandron@yongwww

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
[Bugfix][CMake] Update the minimum CMake version to 3.18 - #12682
Merged
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Sep 29, 2022
Merged
[Bugfix][CMake] Update the minimum CMake version to 3.18#12682
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Conversation
@slyubomirsky
@slyubomirskyslyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to @vinx13. However, the current CMake version check in CMakeLists.txt is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: CUDA_STANDARD is set to invalid value '17'. Upgrading to the latest CMake (3.24) fixed it.
driazati
driazati
approved these changes
Sep 2, 2022
View reviewed changes
@driazatidriazati
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

the cmake docs agree https://cmake.org/cmake/help/latest/prop_tgt/CUDA_STANDARD.html
Comment threadCMakeLists.txt
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.2)
cmake_minimum_required(VERSION 3.18)
@driazatidriazatiSep 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

can you also update the requirements in install/from_source.rst to match?
@slyubomirskyslyubomirskySep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will do, thanks
junrushao
junrushao
approved these changes
Sep 2, 2022
View reviewed changes
@junrushaojunrushao
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Interesting, it looks like the CI's CMake version is below 3.18. Is it not using C++17?
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
Likely nvcc is not used in the CI
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Should the version check be conditional, then? I am not especially familiar with CMake but I imagine it's probably possible.
I am a little confused as to how the CI passes with lower versions of CMake while I had a fresh setup that failed, unless it's not using C++17
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
I don't have much idea how to do such a check if we want to condition whether or not NVCC is used, but it seems possible if we want to warn globally no matter if nvcc is used
@areusch
areusch
commented
Sep 2, 2022

Copy link


Copy Markdown

Contributor
shall we also update https://github.com/apache/tvm/blob/main/docker/install/ubuntu_install_cmake_source.sh?
@driazatidriazati
mentioned this pull request
Sep 2, 2022
 Merged

leandron
leandron
reviewed
Sep 2, 2022
View reviewed changes
@leandronleandron
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@slyubomirsky FYI, in #12131 I'm updating the version we build CMake from source to be compliant with what's being proposed here... 3.18.4 exactly. (cc @driazati)
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Glad to hear the CI's CMake will be updated. I'll also update the install script.
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Ah, I see the last change overlaps with @leandron's PR
yongwww
yongwww
approved these changes
Sep 2, 2022
View reviewed changes
@yongwwwyongwww
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

lgtm
@areusch
areusch
commented
Sep 7, 2022

Copy link


Copy Markdown

Contributor
looks like some issues with the CI, but retriggering as they may be unrelated.
@slyubomirsky
slyubomirsky
commented
Sep 7, 2022

Copy link


Copy Markdown

ContributorAuthor
Is the CI using a higher version of CMake? If not, I think the errors will continue 😅
@driazati
driazati
commented
Sep 7, 2022

Copy link


Copy Markdown

Member
#12131 just merged, we have to update the Docker images to include that before this PR can pass CI
@slyubomirsky
slyubomirsky
commented
Sep 9, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Per this PR, should I make the minimum version higher?
@slyubomirsky
slyubomirsky
commented
Sep 12, 2022

Copy link


Copy Markdown

ContributorAuthor
@driazati when do we expect there to be an update?
@driazati
driazati
commented
Sep 14, 2022

Copy link


Copy Markdown

Member
some of the images were updated to use 3.18 but not all of them, #12774 should clean up the rest so once that is merged and updated on the images this PR should work
@areusch
areusch
commented
Sep 15, 2022

Copy link


Copy Markdown

Contributor
@tvm-bot rerun
@slyubomirsky
slyubomirsky
commented
Sep 22, 2022

Copy link


Copy Markdown

ContributorAuthor
Hm, some of the builds still use different versions of CMake. Any pointers on where the CMake versions for those are defined? I would be glad to make the changes myself.
@driazati
driazati
commented
Sep 27, 2022

Copy link


Copy Markdown

Member
Sorry for the delay, #12906 included a Docker image tag update so they should all be on cmake 3.18+ plus now, a rebase on main should get this PR working
@slyubomirsky
slyubomirsky
commented
Sep 28, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks for the reply. Rebasing as advised
@driazati
driazati
merged commit 8c88aab
into
apache:mainSep 29, 2022
xinetzone pushed a commit
to daobook/tvm
that referenced
this pull request
Nov 25, 2022
@slyubomirsky@xinetzone
[Bugfix][CMake] Update the minimum CMake version to 3.18 (apache#12682)
47d5c44
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to vinx13. However, the current CMake version check in `CMakeLists.txt` is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: `CUDA_STANDARD is set to invalid value '17'`. Upgrading to the latest CMake (3.24) fixed it.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@leandronleandronleandron left review comments
@driazatidriazatidriazati approved these changes
@yongwwwyongwwwyongwww approved these changes
@junrushaojunrushaojunrushao approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@slyubomirsky@junrushao@areusch@driazati@leandron@yongwww

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[Bugfix][CMake] Update the minimum CMake version to 3.18 - #12682
Merged
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Sep 29, 2022
Merged
[Bugfix][CMake] Update the minimum CMake version to 3.18#12682
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Conversation
@slyubomirsky
@slyubomirskyslyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to @vinx13. However, the current CMake version check in CMakeLists.txt is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: CUDA_STANDARD is set to invalid value '17'. Upgrading to the latest CMake (3.24) fixed it.
driazati
driazati
approved these changes
Sep 2, 2022
View reviewed changes
@driazatidriazati
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

the cmake docs agree https://cmake.org/cmake/help/latest/prop_tgt/CUDA_STANDARD.html
Comment threadCMakeLists.txt
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.2)
cmake_minimum_required(VERSION 3.18)
@driazatidriazatiSep 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

can you also update the requirements in install/from_source.rst to match?
@slyubomirskyslyubomirskySep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will do, thanks
junrushao
junrushao
approved these changes
Sep 2, 2022
View reviewed changes
@junrushaojunrushao
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Interesting, it looks like the CI's CMake version is below 3.18. Is it not using C++17?
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
Likely nvcc is not used in the CI
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Should the version check be conditional, then? I am not especially familiar with CMake but I imagine it's probably possible.
I am a little confused as to how the CI passes with lower versions of CMake while I had a fresh setup that failed, unless it's not using C++17
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
I don't have much idea how to do such a check if we want to condition whether or not NVCC is used, but it seems possible if we want to warn globally no matter if nvcc is used
@areusch
areusch
commented
Sep 2, 2022

Copy link


Copy Markdown

Contributor
shall we also update https://github.com/apache/tvm/blob/main/docker/install/ubuntu_install_cmake_source.sh?
@driazatidriazati
mentioned this pull request
Sep 2, 2022
 Merged

leandron
leandron
reviewed
Sep 2, 2022
View reviewed changes
@leandronleandron
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@slyubomirsky FYI, in #12131 I'm updating the version we build CMake from source to be compliant with what's being proposed here... 3.18.4 exactly. (cc @driazati)
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Glad to hear the CI's CMake will be updated. I'll also update the install script.
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Ah, I see the last change overlaps with @leandron's PR
yongwww
yongwww
approved these changes
Sep 2, 2022
View reviewed changes
@yongwwwyongwww
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

lgtm
@areusch
areusch
commented
Sep 7, 2022

Copy link


Copy Markdown

Contributor
looks like some issues with the CI, but retriggering as they may be unrelated.
@slyubomirsky
slyubomirsky
commented
Sep 7, 2022

Copy link


Copy Markdown

ContributorAuthor
Is the CI using a higher version of CMake? If not, I think the errors will continue 😅
@driazati
driazati
commented
Sep 7, 2022

Copy link


Copy Markdown

Member
#12131 just merged, we have to update the Docker images to include that before this PR can pass CI
@slyubomirsky
slyubomirsky
commented
Sep 9, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Per this PR, should I make the minimum version higher?
@slyubomirsky
slyubomirsky
commented
Sep 12, 2022

Copy link


Copy Markdown

ContributorAuthor
@driazati when do we expect there to be an update?
@driazati
driazati
commented
Sep 14, 2022

Copy link


Copy Markdown

Member
some of the images were updated to use 3.18 but not all of them, #12774 should clean up the rest so once that is merged and updated on the images this PR should work
@areusch
areusch
commented
Sep 15, 2022

Copy link


Copy Markdown

Contributor
@tvm-bot rerun
@slyubomirsky
slyubomirsky
commented
Sep 22, 2022

Copy link


Copy Markdown

ContributorAuthor
Hm, some of the builds still use different versions of CMake. Any pointers on where the CMake versions for those are defined? I would be glad to make the changes myself.
@driazati
driazati
commented
Sep 27, 2022

Copy link


Copy Markdown

Member
Sorry for the delay, #12906 included a Docker image tag update so they should all be on cmake 3.18+ plus now, a rebase on main should get this PR working
@slyubomirsky
slyubomirsky
commented
Sep 28, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks for the reply. Rebasing as advised
@driazati
driazati
merged commit 8c88aab
into
apache:mainSep 29, 2022
xinetzone pushed a commit
to daobook/tvm
that referenced
this pull request
Nov 25, 2022
@slyubomirsky@xinetzone
[Bugfix][CMake] Update the minimum CMake version to 3.18 (apache#12682)
47d5c44
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to vinx13. However, the current CMake version check in `CMakeLists.txt` is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: `CUDA_STANDARD is set to invalid value '17'`. Upgrading to the latest CMake (3.24) fixed it.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@leandronleandronleandron left review comments
@driazatidriazatidriazati approved these changes
@yongwwwyongwwwyongwww approved these changes
@junrushaojunrushaojunrushao approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@slyubomirsky@junrushao@areusch@driazati@leandron@yongwww

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[Bugfix][CMake] Update the minimum CMake version to 3.18 - #12682
Merged
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Sep 29, 2022
Merged
[Bugfix][CMake] Update the minimum CMake version to 3.18#12682
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Conversation
@slyubomirsky
@slyubomirskyslyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to @vinx13. However, the current CMake version check in CMakeLists.txt is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: CUDA_STANDARD is set to invalid value '17'. Upgrading to the latest CMake (3.24) fixed it.
driazati
driazati
approved these changes
Sep 2, 2022
View reviewed changes
@driazatidriazati
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

the cmake docs agree https://cmake.org/cmake/help/latest/prop_tgt/CUDA_STANDARD.html
Comment threadCMakeLists.txt
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.2)
cmake_minimum_required(VERSION 3.18)
@driazatidriazatiSep 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

can you also update the requirements in install/from_source.rst to match?
@slyubomirskyslyubomirskySep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will do, thanks
junrushao
junrushao
approved these changes
Sep 2, 2022
View reviewed changes
@junrushaojunrushao
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Interesting, it looks like the CI's CMake version is below 3.18. Is it not using C++17?
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
Likely nvcc is not used in the CI
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Should the version check be conditional, then? I am not especially familiar with CMake but I imagine it's probably possible.
I am a little confused as to how the CI passes with lower versions of CMake while I had a fresh setup that failed, unless it's not using C++17
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
I don't have much idea how to do such a check if we want to condition whether or not NVCC is used, but it seems possible if we want to warn globally no matter if nvcc is used
@areusch
areusch
commented
Sep 2, 2022

Copy link


Copy Markdown

Contributor
shall we also update https://github.com/apache/tvm/blob/main/docker/install/ubuntu_install_cmake_source.sh?
@driazatidriazati
mentioned this pull request
Sep 2, 2022
 Merged

leandron
leandron
reviewed
Sep 2, 2022
View reviewed changes
@leandronleandron
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@slyubomirsky FYI, in #12131 I'm updating the version we build CMake from source to be compliant with what's being proposed here... 3.18.4 exactly. (cc @driazati)
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Glad to hear the CI's CMake will be updated. I'll also update the install script.
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Ah, I see the last change overlaps with @leandron's PR
yongwww
yongwww
approved these changes
Sep 2, 2022
View reviewed changes
@yongwwwyongwww
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

lgtm
@areusch
areusch
commented
Sep 7, 2022

Copy link


Copy Markdown

Contributor
looks like some issues with the CI, but retriggering as they may be unrelated.
@slyubomirsky
slyubomirsky
commented
Sep 7, 2022

Copy link


Copy Markdown

ContributorAuthor
Is the CI using a higher version of CMake? If not, I think the errors will continue 😅
@driazati
driazati
commented
Sep 7, 2022

Copy link


Copy Markdown

Member
#12131 just merged, we have to update the Docker images to include that before this PR can pass CI
@slyubomirsky
slyubomirsky
commented
Sep 9, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Per this PR, should I make the minimum version higher?
@slyubomirsky
slyubomirsky
commented
Sep 12, 2022

Copy link


Copy Markdown

ContributorAuthor
@driazati when do we expect there to be an update?
@driazati
driazati
commented
Sep 14, 2022

Copy link


Copy Markdown

Member
some of the images were updated to use 3.18 but not all of them, #12774 should clean up the rest so once that is merged and updated on the images this PR should work
@areusch
areusch
commented
Sep 15, 2022

Copy link


Copy Markdown

Contributor
@tvm-bot rerun
@slyubomirsky
slyubomirsky
commented
Sep 22, 2022

Copy link


Copy Markdown

ContributorAuthor
Hm, some of the builds still use different versions of CMake. Any pointers on where the CMake versions for those are defined? I would be glad to make the changes myself.
@driazati
driazati
commented
Sep 27, 2022

Copy link


Copy Markdown

Member
Sorry for the delay, #12906 included a Docker image tag update so they should all be on cmake 3.18+ plus now, a rebase on main should get this PR working
@slyubomirsky
slyubomirsky
commented
Sep 28, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks for the reply. Rebasing as advised
@driazati
driazati
merged commit 8c88aab
into
apache:mainSep 29, 2022
xinetzone pushed a commit
to daobook/tvm
that referenced
this pull request
Nov 25, 2022
@slyubomirsky@xinetzone
[Bugfix][CMake] Update the minimum CMake version to 3.18 (apache#12682)
47d5c44
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to vinx13. However, the current CMake version check in `CMakeLists.txt` is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: `CUDA_STANDARD is set to invalid value '17'`. Upgrading to the latest CMake (3.24) fixed it.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@leandronleandronleandron left review comments
@driazatidriazatidriazati approved these changes
@yongwwwyongwwwyongwww approved these changes
@junrushaojunrushaojunrushao approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@slyubomirsky@junrushao@areusch@driazati@leandron@yongwww

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
[Bugfix][CMake] Update the minimum CMake version to 3.18 - #12682
Merged
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Sep 29, 2022
Merged
[Bugfix][CMake] Update the minimum CMake version to 3.18#12682
driazati merged 2 commits into
apache:mainfrom 
slyubomirsky:cmakelist-min-version
Conversation
@slyubomirsky
@slyubomirskyslyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to @vinx13. However, the current CMake version check in CMakeLists.txt is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: CUDA_STANDARD is set to invalid value '17'. Upgrading to the latest CMake (3.24) fixed it.
driazati
driazati
approved these changes
Sep 2, 2022
View reviewed changes
@driazatidriazati
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

the cmake docs agree https://cmake.org/cmake/help/latest/prop_tgt/CUDA_STANDARD.html
Comment threadCMakeLists.txt
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.2)
cmake_minimum_required(VERSION 3.18)
@driazatidriazatiSep 2, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

can you also update the requirements in install/from_source.rst to match?
@slyubomirskyslyubomirskySep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will do, thanks
junrushao
junrushao
approved these changes
Sep 2, 2022
View reviewed changes
@junrushaojunrushao
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Interesting, it looks like the CI's CMake version is below 3.18. Is it not using C++17?
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
Likely nvcc is not used in the CI
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Should the version check be conditional, then? I am not especially familiar with CMake but I imagine it's probably possible.
I am a little confused as to how the CI passes with lower versions of CMake while I had a fresh setup that failed, unless it's not using C++17
@junrushao
junrushao
commented
Sep 2, 2022

Copy link


Copy Markdown

Member
I don't have much idea how to do such a check if we want to condition whether or not NVCC is used, but it seems possible if we want to warn globally no matter if nvcc is used
@areusch
areusch
commented
Sep 2, 2022

Copy link


Copy Markdown

Contributor
shall we also update https://github.com/apache/tvm/blob/main/docker/install/ubuntu_install_cmake_source.sh?
@driazatidriazati
mentioned this pull request
Sep 2, 2022
 Merged

leandron
leandron
reviewed
Sep 2, 2022
View reviewed changes
@leandronleandron
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@slyubomirsky FYI, in #12131 I'm updating the version we build CMake from source to be compliant with what's being proposed here... 3.18.4 exactly. (cc @driazati)
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Glad to hear the CI's CMake will be updated. I'll also update the install script.
@slyubomirsky
slyubomirsky
commented
Sep 2, 2022

Copy link


Copy Markdown

ContributorAuthor
Ah, I see the last change overlaps with @leandron's PR
yongwww
yongwww
approved these changes
Sep 2, 2022
View reviewed changes
@yongwwwyongwww
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

lgtm
@areusch
areusch
commented
Sep 7, 2022

Copy link


Copy Markdown

Contributor
looks like some issues with the CI, but retriggering as they may be unrelated.
@slyubomirsky
slyubomirsky
commented
Sep 7, 2022

Copy link


Copy Markdown

ContributorAuthor
Is the CI using a higher version of CMake? If not, I think the errors will continue 😅
@driazati
driazati
commented
Sep 7, 2022

Copy link


Copy Markdown

Member
#12131 just merged, we have to update the Docker images to include that before this PR can pass CI
@slyubomirsky
slyubomirsky
commented
Sep 9, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Per this PR, should I make the minimum version higher?
@slyubomirsky
slyubomirsky
commented
Sep 12, 2022

Copy link


Copy Markdown

ContributorAuthor
@driazati when do we expect there to be an update?
@driazati
driazati
commented
Sep 14, 2022

Copy link


Copy Markdown

Member
some of the images were updated to use 3.18 but not all of them, #12774 should clean up the rest so once that is merged and updated on the images this PR should work
@areusch
areusch
commented
Sep 15, 2022

Copy link


Copy Markdown

Contributor
@tvm-bot rerun
@slyubomirsky
slyubomirsky
commented
Sep 22, 2022

Copy link


Copy Markdown

ContributorAuthor
Hm, some of the builds still use different versions of CMake. Any pointers on where the CMake versions for those are defined? I would be glad to make the changes myself.
@driazati
driazati
commented
Sep 27, 2022

Copy link


Copy Markdown

Member
Sorry for the delay, #12906 included a Docker image tag update so they should all be on cmake 3.18+ plus now, a rebase on main should get this PR working
@slyubomirsky
slyubomirsky
commented
Sep 28, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks for the reply. Rebasing as advised
@driazati
driazati
merged commit 8c88aab
into
apache:mainSep 29, 2022
xinetzone pushed a commit
to daobook/tvm
that referenced
this pull request
Nov 25, 2022
@slyubomirsky@xinetzone
[Bugfix][CMake] Update the minimum CMake version to 3.18 (apache#12682)
47d5c44
TVM has recently switched to C++17. CUDA support with C++17 requires CMake past version 3.18, according to vinx13. However, the current CMake version check in `CMakeLists.txt` is not checking for a sufficiently high CMake version; this PR updates it.
Bug that prompted this: When building with CMake version 3.16 I had the following error: `CUDA_STANDARD is set to invalid value '17'`. Upgrading to the latest CMake (3.24) fixed it.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@leandronleandronleandron left review comments
@driazatidriazatidriazati approved these changes
@yongwwwyongwwwyongwww approved these changes
@junrushaojunrushaojunrushao approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@slyubomirsky@junrushao@areusch@driazati@leandron@yongwww