This is an archive of the discontinued LLVM Phabricator instance.

Functionality Added to CMAKE
AbandonedPublic

Authored by ps-19 on Mar 31 2022, 6:58 AM.

Details

Summary

Added functionality to CMAKE file. Added NONE as type to CMAKE_RELEASE.

Diff Detail

Event Timeline

ps-19 created this revision.Mar 31 2022, 6:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2022, 6:58 AM
Herald added a subscriber: mgorny. · View Herald Transcript
ps-19 requested review of this revision.Mar 31 2022, 6:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2022, 6:58 AM
ps-19 updated this revision to Diff 419555.Mar 31 2022, 1:18 PM

Formatted again using Clang-Format.

ps-19 added a comment.EditedMar 31 2022, 10:12 PM

Can any one please specify the issue in building? I am not being able to understand.

Can any one please specify the issue in building? I am not being able to understand.

The Debian failures are unrelated to your patch.

Can you explain what you're trying to accomplish with this patch? I don't think we want a NONE category for the build type as that doesn't make sense to me -- what does it mean to have no build type?

ps-19 added a comment.Apr 1 2022, 5:48 AM

i have seen some cases in repos where no build variants are found and that creates error in building at some moment. It's good standard practice to include NONE type even we don't need this time, else everything is fine for now, i may close the patch for now.

i have seen some cases in repos where no build variants are found and that creates error in building at some moment. It's good standard practice to include NONE type even we don't need this time, else everything is fine for now, i may close the patch for now.

I don't know enough about CMake best practices to know whether this is or isn't a standard practice (it's not from what I know, but I don't know much in this space). Personally, I think if the user ends up with "none" as the build type, an error is a kindness for them because something's gone weird in the build system. Adding a few more reviewers in case I'm wrong though.

Looping in a few others with CMake experience.

NONE isn't really a CMAKE_BUILD_TYPE in the normal flow (see: https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html), and it would have undefined behavior inside LLVM because we key quite a few things off the CMAKE_BUILD_TYPE, so I'm a little concerned about this change.

I'm not sure why you think supporting NONE is a good practice, my opinion is that an undefined build type would be a bad thing to support...

Looping in a few others with CMake experience.

NONE isn't really a CMAKE_BUILD_TYPE in the normal flow (see: https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html), and it would have undefined behavior inside LLVM because we key quite a few things off the CMAKE_BUILD_TYPE, so I'm a little concerned about this change.

I'm not sure why you think supporting NONE is a good practice, my opinion is that an undefined build type would be a bad thing to support...

I agree, I haven't yet encountered NONE in CMake projects and it's not clear to me why we should support it in LLVM.

smeenai requested changes to this revision.Apr 28 2022, 1:41 PM

Requesting changes since there are unaddressed reviewer questions.

This revision now requires changes to proceed.Apr 28 2022, 1:41 PM
ldionne resigned from this revision.Jun 10 2022, 6:37 AM
ldionne added a subscriber: ldionne.

(This should be abandoned if no work is happening on this to clear up everybody's review queues)

ps-19 abandoned this revision.Jun 10 2022, 7:23 AM