Make CMake work out of the box #970
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dscho
reviewed
Jun 4, 2021
contrib/buildsystems/CMakeLists.txt
Outdated
| set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe) | ||
| if (USING_VCPKG) | ||
| set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe) | ||
| endif() |
There was a problem hiding this comment.
While you are already in this area, something I noticed a while ago is that we're trying to set MSGFMT_EXE even if NO_GETTEXT was configured, which does not make sense.
Maybe I can talk you into ensuring that MSGFMT_EXE is unset in that case, e.g. by surrounding this entire block with:
if(NO_GETTEXT)
if(MSGFMT_EXE)
message(WARNING "MSGFMT_EXE ignored under NO_GETTEXT")
unset(MSGFMT_EXE)
endif()
else()
... <current block> ...
endif()There was a problem hiding this comment.
I've added in the code as you've asked, I'm not super familiar with how its supposed to act with the Intl library, since if that gets found it sets NO_GETTEXT to 1 and it forces NO_GETTEXT to 1 later in the code. So I'm not sure if it's ever actually possible to have Intl=1 and NO_GETTEXT=0 (or unset or whatever) at the same time.
dscho
reviewed
Jun 4, 2021
contrib/buildsystems/CMakeLists.txt
Outdated
| message(WARNING "Text Translations won't be built") | ||
| unset(MSGFMT_EXE) | ||
| if(NO_GETEXT) | ||
| if(MSGFMT_EXE) |
There was a problem hiding this comment.
I fear that this is dead code, given that we're inside a top-level if(NOT MSGFMT_EXE) 😀
| if(NOT EXISTS ${MSGFMT_EXE}) | ||
| message(WARNING "Text Translations won't be built") | ||
| unset(MSGFMT_EXE) | ||
| endif() |
There was a problem hiding this comment.
There is a missing endif() here, I think, for the else() above.
And I'd like to avoid find_program(MSGFMT_EXE) altogether if NO_GETTEXT is set...
There was a problem hiding this comment.
I made some updates with your comments in mind, I think the version I have is a bit more what we want, I changed the text output from a warning to a status because this should be normal operation if we're operating under NO_GETTEXT and I felt issuing a warning for that would be a bit weird.
|
/submit |
|
Submitted as pull.970.git.1622828605.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -43,14 +43,24 @@ NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studi | |||
| to use another tool say `ninja` add this to the command line when configuring. | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Fri, Jun 4, 2021 at 1:44 PM Matthew Rogers via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> When building on windows users have the option to use vcpkg to provide
> the dependencies needed to compile. Previously, this was used only when
> using the Visual Studio generator which was not ideal because:
>
> - Not all users who want to use vcpkg use the Visual Studio
> generators.
>
> - Some versions of Visual Studio 2019 moved away from using the
> VS 2019 by default, making it impossible for Visual Studio to
> configure the project in the likely event that it couldn't find the
> dependencies.
Is there something missing between "using the" and "VS 2019"? I'm
having a hard time trying to understand what this bullet point is
saying due to this apparent gap.
> - Inexperienced users of CMake are very likely to get tripped up by
> the errors caused by a lack of vcpkg, making the above bullet point
> both annoying and hard to debug.
>
> As such, lets make using vcpkg the default on windows. Users who want
> to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE.
s/lets/let's/
> Signed-off-by: Matthew Rogers <mattr94@gmail.com>
There was a problem hiding this comment.
On the Git mailing list, Matt Rogers wrote (reply to this):
On Fri, Jun 4, 2021 at 2:03 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Jun 4, 2021 at 1:44 PM Matthew Rogers via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > When building on windows users have the option to use vcpkg to provide
> > the dependencies needed to compile. Previously, this was used only when
> > using the Visual Studio generator which was not ideal because:
> >
> > - Not all users who want to use vcpkg use the Visual Studio
> > generators.
> >
> > - Some versions of Visual Studio 2019 moved away from using the
> > VS 2019 by default, making it impossible for Visual Studio to
> > configure the project in the likely event that it couldn't find the
> > dependencies.
>
> Is there something missing between "using the" and "VS 2019"? I'm
> having a hard time trying to understand what this bullet point is
> saying due to this apparent gap.
>
Yeah, this should really read
- Some versions of Visual Studio 2019 moved away from using the
VS 2019 _Generator_ by default, making it impossible for Visual Studio to
configure the project in the likely event that it couldn't find the
dependencies.
> > - Inexperienced users of CMake are very likely to get tripped up by
> > the errors caused by a lack of vcpkg, making the above bullet point
> > both annoying and hard to debug.
> >
> > As such, lets make using vcpkg the default on windows. Users who want
> > to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE.
>
> s/lets/let's/
>
> > Signed-off-by: Matthew Rogers <mattr94@gmail.com>
--
Matthew Rogers
|
User |
| @@ -43,14 +43,28 @@ NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studi | |||
| to use another tool say `ninja` add this to the command line when configuring. | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Fri, Jun 4, 2021 at 1:44 PM Matthew Rogers via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Some users have expressed interest in a more "batteries included" way of
> building via CMake[1], and a big part of that is providing easier access
> to tooling external tools.
>
> A straightforward way to accomplish this is to make it as simple as
> possible is to enable the generation of the compile_commands.json file,
> which is supported by many tools such as: clang-tidy, clang-format,
> sourcetrail, etc.
>
> This does come with a small run-time overhead during the configuration
> step (~6 seconds on my machine):
> [...]
> This seems like a small enough price to pay to make the project more
> accessible to newer users. Additionally there are other large projects
> like llvm [2] which has had this enabled by default for >6 years at the
> time of this writing, and no real negative consequences that I can find
> with my search-skills.
>
> NOTE: That the comppile_commands.json is currenntly produced only when
> using the Ninja and Makefile generators. See The CMake documentation[3]
> for more info.
s/comppile/compile/
s/currenntly/currently/
> Signed-off-by: Matthew Rogers <mattr94@gmail.com>
| @@ -43,14 +43,28 @@ NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studi | |||
| to use another tool say `ninja` add this to the command line when configuring. | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Fri, Jun 4, 2021 at 1:44 PM Matthew Rogers via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> It does not make sense to attempt to set MSGFMT_EXE when NO_GETTEXT is
> configured, as such add a check for NO_GETTEXT before attempting to set
> it.
This would be easier to digest if "as such" is the start of a new
sentence: "As such...". Or "Therefore, add a check...".
> suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Matthew Rogers <mattr94@gmail.com>
s/suggested-by/Suggested-by:/
Tiny little nits, both. Don't know if it's worth a re-roll, but if you
happen to re-roll for some other reason, perhaps these could be
tweaked.
| @@ -43,14 +43,24 @@ NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studi | |||
| to use another tool say `ninja` add this to the command line when configuring. | |||
There was a problem hiding this comment.
On the Git mailing list, Sibi Siddharthan wrote (reply to this):
On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> -if(WIN32)
> +
> +if (WIN32 AND NOT NO_VCPKG)
> + set(USING_VCPKG TRUE)
> +else()
> + set(USING_VCPKG FALSE)
> +endif()
I think it would be better if we could have an option for this knob.
Maybe like this
option(NO_VCPKG "Don't use vcpkg for obtaining dependencies. Only
applicable to Windows platforms" OFF)
I would prefer to use `USE_VCPKG`.
Thank You,
Sibi Siddharthan
There was a problem hiding this comment.
On the Git mailing list, Matt Rogers wrote (reply to this):
On Fri, Jun 4, 2021 at 4:55 PM Sibi Siddharthan
<sibisiddharthan.github@gmail.com> wrote:
>
> On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>
> > -if(WIN32)
> > +
> > +if (WIN32 AND NOT NO_VCPKG)
> > + set(USING_VCPKG TRUE)
> > +else()
> > + set(USING_VCPKG FALSE)
> > +endif()
>
> I think it would be better if we could have an option for this knob.
> Maybe like this
>
> option(NO_VCPKG "Don't use vcpkg for obtaining dependencies. Only
> applicable to Windows platforms" OFF)
Option would definitely be the better tool to use here, I just didn't
think about
it when originally writing it, so I'll send a reroll with that and the spelling
corrections suggested by Eric Sunshine. I assume you'd prefer something
with a final form more like:
option(USE_VCPKG "Whether or not to use vcpkg for obtaining dependencies.
Only applicable to Windows platforms" ON)
>
> I would prefer to use `USE_VCPKG`.
>
> Thank You,
> Sibi Siddharthan
--
Matthew Rogers
There was a problem hiding this comment.
On the Git mailing list, Sibi Siddharthan wrote (reply to this):
On Sun, Jun 6, 2021 at 4:01 AM Matt Rogers <mattr94@gmail.com> wrote:
>
> On Fri, Jun 4, 2021 at 4:55 PM Sibi Siddharthan
> <sibisiddharthan.github@gmail.com> wrote:
> >
> > On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >
> > > -if(WIN32)
> > > +
> > > +if (WIN32 AND NOT NO_VCPKG)
> > > + set(USING_VCPKG TRUE)
> > > +else()
> > > + set(USING_VCPKG FALSE)
> > > +endif()
> >
> > I think it would be better if we could have an option for this knob.
> > Maybe like this
> >
> > option(NO_VCPKG "Don't use vcpkg for obtaining dependencies. Only
> > applicable to Windows platforms" OFF)
>
> Option would definitely be the better tool to use here, I just didn't
> think about
> it when originally writing it, so I'll send a reroll with that and the spelling
> corrections suggested by Eric Sunshine. I assume you'd prefer something
> with a final form more like:
>
> option(USE_VCPKG "Whether or not to use vcpkg for obtaining dependencies.
> Only applicable to Windows platforms" ON)
>
Yes, this would be better.
Thank You,
Sibi Siddharthan
|
User |
| @@ -43,14 +43,28 @@ NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studi | |||
| to use another tool say `ninja` add this to the command line when configuring. | |||
There was a problem hiding this comment.
On the Git mailing list, Sibi Siddharthan wrote (reply to this):
On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> A straightforward way to accomplish this is to make it as simple as
> possible is to enable the generation of the compile_commands.json file,
> which is supported by many tools such as: clang-tidy, clang-format,
> sourcetrail, etc.
>
> This does come with a small run-time overhead during the configuration
> step (~6 seconds on my machine):
>
> Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=TRUE
>
> real 1m9.840s
> user 0m0.031s
> sys 0m0.031s
>
> Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=FALSE
>
> real 1m3.195s
> user 0m0.015s
> sys 0m0.015s
>
> This seems like a small enough price to pay to make the project more
> accessible to newer users. Additionally there are other large projects
> like llvm [2] which has had this enabled by default for >6 years at the
> time of this writing, and no real negative consequences that I can find
> with my search-skills.
>
The overhead is actually much smaller than that. In my system it is
less than 150ms.
The first configure takes this long because we generate command-list.h
and config-list.h.
This process is really slow under Windows.
Thank You,
Sibi Siddharthan
There was a problem hiding this comment.
On the Git mailing list, Matt Rogers wrote (reply to this):
On Fri, Jun 4, 2021 at 5:09 PM Sibi Siddharthan
<sibisiddharthan.github@gmail.com> wrote:
>
> On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > A straightforward way to accomplish this is to make it as simple as
> > possible is to enable the generation of the compile_commands.json file,
> > which is supported by many tools such as: clang-tidy, clang-format,
> > sourcetrail, etc.
> >
> > This does come with a small run-time overhead during the configuration
> > step (~6 seconds on my machine):
> >
> > Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=TRUE
> >
> > real 1m9.840s
> > user 0m0.031s
> > sys 0m0.031s
> >
> > Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=FALSE
> >
> > real 1m3.195s
> > user 0m0.015s
> > sys 0m0.015s
> >
> > This seems like a small enough price to pay to make the project more
> > accessible to newer users. Additionally there are other large projects
> > like llvm [2] which has had this enabled by default for >6 years at the
> > time of this writing, and no real negative consequences that I can find
> > with my search-skills.
> >
>
> The overhead is actually much smaller than that. In my system it is
> less than 150ms.
Is that 150 ms for the whole process or just the difference between the two
options? I'm running this on windows via the git bash provided by the
git sdk.
> The first configure takes this long because we generate command-list.h
> and config-list.h.
> This process is really slow under Windows.
>
I used two different build directories for both my invocations specifically
to avoid having to account for cache variables and other side effects
from earlier configurations. The variation could also be from network
latency since in this test I was downloading vcpkg, etc.
> Thank You,
> Sibi Siddharthan
--
Matthew Rogers
There was a problem hiding this comment.
On the Git mailing list, Sibi Siddharthan wrote (reply to this):
On Sun, Jun 6, 2021 at 4:06 AM Matt Rogers <mattr94@gmail.com> wrote:
>
> On Fri, Jun 4, 2021 at 5:09 PM Sibi Siddharthan
> <sibisiddharthan.github@gmail.com> wrote:
> >
> > On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >
> > > A straightforward way to accomplish this is to make it as simple as
> > > possible is to enable the generation of the compile_commands.json file,
> > > which is supported by many tools such as: clang-tidy, clang-format,
> > > sourcetrail, etc.
> > >
> > > This does come with a small run-time overhead during the configuration
> > > step (~6 seconds on my machine):
> > >
> > > Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=TRUE
> > >
> > > real 1m9.840s
> > > user 0m0.031s
> > > sys 0m0.031s
> > >
> > > Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=FALSE
> > >
> > > real 1m3.195s
> > > user 0m0.015s
> > > sys 0m0.015s
> > >
> > > This seems like a small enough price to pay to make the project more
> > > accessible to newer users. Additionally there are other large projects
> > > like llvm [2] which has had this enabled by default for >6 years at the
> > > time of this writing, and no real negative consequences that I can find
> > > with my search-skills.
> > >
> >
> > The overhead is actually much smaller than that. In my system it is
> > less than 150ms.
>
> Is that 150 ms for the whole process or just the difference between the two
> options? I'm running this on windows via the git bash provided by the
> git sdk.
The difference between the two. Without exporting compile_commands.json
it takes around 650ms, with it around 750ms.
NOTE: This is for subsequent CMake runs. (Excludes the initial run)
Thank You,
Sibi Siddharthan
|
On the Git mailing list, Bagas Sanjaya wrote (reply to this): |
|
User |
|
On the Git mailing list, Matt Rogers wrote (reply to this): |
When building on windows users have the option to use vcpkg to provide
the dependencies needed to compile. Previously, this was used only when
using the Visual Studio generator which was not ideal because:
- Not all users who want to use vcpkg use the Visual Studio
generators.
- Some versions of Visual Studio 2019 moved away from using the
VS 2019 generator by default, making it impossible for Visual
Studio to configure the project in the likely event that it couldn't
find the dependencies.
- Inexperienced users of CMake are very likely to get tripped up by
the errors caused by a lack of vcpkg, making the above bullet point
both annoying and hard to debug.
As such, let's make using vcpkg the default on windows. Users who want
to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE.
Signed-off-by: Matthew Rogers <mattr94@gmail.com>
Some users have expressed interest in a more "batteries included" way of
building via CMake[1], and a big part of that is providing easier access
to tooling external tools.
A straightforward way to accomplish this is to make it as simple as
possible is to enable the generation of the compile_commands.json file,
which is supported by many tools such as: clang-tidy, clang-format,
sourcetrail, etc.
This does come with a small run-time overhead during the configuration
step (~6 seconds on my machine):
Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=TRUE
real 1m9.840s
user 0m0.031s
sys 0m0.031s
Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=FALSE
real 1m3.195s
user 0m0.015s
sys 0m0.015s
This seems like a small enough price to pay to make the project more
accessible to newer users. Additionally there are other large projects
like llvm [2] which has had this enabled by default for >6 years at the
time of this writing, and no real negative consequences that I can find
with my search-skills.
NOTE: That the compile_commands.json is currently produced only when
using the Ninja and Makefile generators. See The CMake documentation[3]
for more info.
1: https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/
2: llvm/llvm-project@2c57120
3: https://cmake.org/cmake/help/latest/variable/CMAKE_EXPORT_COMPILE_COMMANDS.html
Signed-off-by: Matthew Rogers <mattr94@gmail.com>
It does not make sense to attempt to set MSGFMT_EXE when NO_GETTEXT is configured, as such add a check for NO_GETTEXT before attempting to set it. Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Matthew Rogers <mattr94@gmail.com>
|
/submit |
|
Submitted as pull.970.v2.git.1622980974.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
This branch is now known as |
|
This patch series was integrated into seen via git@285c226. |
|
This patch series was integrated into seen via git@53c014d. |
|
There was a status update in the "New Topics" section about the branch CMake update. Will merge to 'next'. |
|
This patch series was integrated into seen via git@79d4d58. |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
This patch series was integrated into seen via git@b1f2824. |
|
This patch series was integrated into seen via git@8ac3892. |
|
There was a status update in the "Cooking" section about the branch CMake update. Will merge to 'next'. |
|
This patch series was integrated into seen via git@0154f4d. |
|
There was a status update in the "Cooking" section about the branch CMake update. Will merge to 'next'. |
|
On the Git mailing list, Philip Oakley wrote (reply to this): |
|
On the Git mailing list, Philip Oakley wrote (reply to this): |
|
On the Git mailing list, Philip Oakley wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Philip Oakley wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
This patch series was integrated into seen via git@9b4694b. |
|
There was a status update in the "Cooking" section about the branch CMake update. Will merge to 'next'. |
|
This patch series was integrated into seen via git@a88e0e7. |
|
This patch series was integrated into seen via git@c0f9c0a. |
|
There was a status update in the "Cooking" section about the branch CMake update. Will merge to 'next'. |
|
This patch series was integrated into seen via git@4dc96c1. |
|
This patch series was integrated into next via git@efc86eb. |
|
This patch series was integrated into seen via git@c37a60b. |
|
There was a status update in the "Cooking" section about the branch CMake update. Will merge to 'master'. |
|
This patch series was integrated into seen via git@c3c0b71. |
|
This patch series was integrated into next via git@c3c0b71. |
|
This patch series was integrated into master via git@c3c0b71. |
|
Closed via c3c0b71. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request comes from our discussion here[1], and I think these patches provide a good compromise around the concerns discussed there
1: https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/
CCing the people involved in the original discussion.
cc: Philip Oakley philipoakley@iee.email
cc: Sibi Siddharthan sibisiddharthan.github@gmail.com,
cc: Johannes Schindelin johannes.schindelin@gmx.de,
cc: Danh Doan congdanhqx@gmail.com
cc: Eric Sunshine sunshine@sunshineco.com
cc: Bagas Sanjaya bagasdotme@gmail.com