Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cmake: use ENV{VCPKG_ROOT}, fix msgfmt + minor. #3685

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

rkitover
Copy link

@rkitover rkitover commented Feb 4, 2022

Set VCPKG_DIR from ENV{VCPKG_ROOT} if set, otherwise set
ENV{VCPKG_ROOT}.
Set VCPKG_ARCH from VCPKG_TARGET_TRIPLET if set.
Fix check for VCPKG_ARCH being defined.
Describe options in comment at the top of the file.
Add build*/ out-of-source build dirs to toplevel .gitignore.
Change vcpkg_install.bat to use %VCPKG_ROOT% set by cmake or present in
the environment, and update/install only what's necessary.
Use Gettext.Tools from nuget on Windows for msgfmt.
Fix cmake hardlinking script invocation by adding WORKING_DIRECTORY.

Signed-off-by: Rafael Kitover [email protected]

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, it's a lot of changes!

Also, those changes that are logically separate from each other should be packed into their own commit, each with a commit message that motivates the change. In other words, this commit should be split into multiple commits.

May I ask you to do that, and then force-push?

@rkitover
Copy link
Author

rkitover commented Feb 4, 2022

No problem, I will do that.

@PhilipOakley
Copy link

It looks like the commit could be split by

  1. the indentation of the IF (Git not found)
  2. the initial DEFINED VCPKG_ROOT checking
  3. the flip to using VCPKG_ROOT
  4. the VCPKG_ARCH steps
  5. the msgfmt and nuget steps (inc that nuget offers..)

The commit messages themselves should include a statement of the problem that is being solved, e.g. that devs cannot use their your own VCPKG tree... (borrowed from the cmake comment).

Looks good.

@rkitover rkitover force-pushed the cmake-improvements branch 2 times, most recently from ebd1ae9 to 2abf6a4 Compare February 6, 2022 19:52
@dscho
Copy link
Member

dscho commented Feb 6, 2022

Hmm. I don't see that the force-pushed versions separate the individual concerns properly. Instead, I see even more concerns crammed into that poor single commit. That's kind of the diametrical opposite of what I would like to see.

@rkitover
Copy link
Author

rkitover commented Feb 6, 2022

Sorry, I am trying to do some cleanups before I separate the commits. Then I will do a bisect and make sure they all work individually.

@dscho
Copy link
Member

dscho commented Feb 6, 2022

I am trying to do some cleanups

I see that you added two non-trivial CMake files, and cannot help but wonder why we would need those. It does not strike me as a clean-up to add them when the code before that seemed to work all right.

And before any clean-ups, I would have hoped that the relatively consistent (and therefore probably not flaky) test failures would be addressed. See e.g. https://github.com/git-for-windows/git/runs/5085425240?check_suite_focus=true#step:6:862

@rkitover
Copy link
Author

rkitover commented Feb 6, 2022

There are no changes to the source code or any of the libraries used, I don't see how any git test failures could be related to this. But I will compare to master to make sure.

@rkitover
Copy link
Author

rkitover commented Feb 6, 2022

Any way, please give me a few more days.

@dscho
Copy link
Member

dscho commented Feb 6, 2022

Any way, please give me a few more days.

Sure, but why trigger new builds by these frequent force-pushes, unless you want people to see what you pushed?

@dscho
Copy link
Member

dscho commented Feb 6, 2022

You might also want to convert the PR to a draft, to indicate that it's not ready for review.

@rkitover rkitover marked this pull request as draft February 6, 2022 22:41
@rkitover
Copy link
Author

rkitover commented Feb 6, 2022

No problem, I have converted to draft and I will push to a different branch until it's ready.

@rkitover rkitover requested a review from dscho February 10, 2022 15:37
@rkitover rkitover marked this pull request as ready for review February 10, 2022 15:37
@rkitover
Copy link
Author

@dscho @PhilipOakley

I've split the commits and I ran this script with this data file to check that they all work with a standard build.

check_commits.ps1

$erroractionpreference = 'stop'

foreach($commit in (gc commits.txt)) {
    $build_dir = "build-$commit"

    git reset --hard $commit

    ri -r $build_dir -ea ignore
    mkdir $build_dir | out-null

    pushd $build_dir

    cmake ../contrib/buildsystems -G Ninja
    ninja

    if (-not (test-path git.exe)) {
        write-error "Building $commit failed." -ea stop
    }

    popd
}

"ALL COMMITS BUILT SUCCESSFULLY"

commits.txt

007ec82335
7609d16562
5fc1f867a1
038021aa79
f86b59bfaf
f012bee432
d86a13b871
9a1520e870
ce892f8f0b
0c5cb9f661
ab80c468f9
fc219be592
6abf886de4
580397f0a2
2f4c48c77b
a21b05fbcc
319740f140
d7901d9651

@PhilipOakley
Copy link

007ec82335
7609d16562
5fc1f867a1
038021aa79
f86b59bfaf
f012bee432
d86a13b871
9a1520e870
ce892f8f0b
0c5cb9f661
ab80c468f9
fc219be592
6abf886de4
580397f0a2
2f4c48c77b
a21b05fbcc
319740f140
d7901d9651

Hi Rafael,
The split generally looks good. I did have a few small comments, some of which are based on the main Git guidelines to keep consistency.

I fetched your changes via git fetch https://github.com/rkitover/git.git cmake-improvements:cmake-improvements and viewed them via gitk rather than using the GitHub interface.

  1. the commit subject lines should omit the period. no full stop
  2. Try not to reference previous commits by sha1, or sha1 alone. It's better to say 'previous commit', (cf 'subsequent commit') as the sha1 may change if there are edits, or they are submitted upstream as patches. The fallback is to use the log --pretty=reference to get the subject and date stamp as well. commit-reference

A few specifics

  1. 5fc1f86 (Fix check for built vcpkg in vcpkg_install.bat.) - The 'some circumstances' - giving one example would help (just one is enough).
  2. 0c5cb9 (Remove messages for uninitialized cmake vars) - those were from previous 'debug' sessions to make the achieved setup plain to the user. Dropping those is OK.
  3. 6abf88 (Always call vcpkg_install.bat from cmake.) - Hmm. Is the message still suitable. Is a similar message already in vcpkg_install.bat? Does some other 'if' (not visible in the diff) already cover this? Worth clarifying in the message.
  4. 580397(Add build_option() to cmake/Utilities.cmake.) - "This function performs does nearly the same thing as the builtin cmake" doesn't scan well, repetition? The description didn't flow that well - I have difficulty understanding what is being said in that sentence. Maybe the msg also needs "This function will be used in a subsequent commit".
    Don't assume the reader will remember the subject line while reading these longer commit messages, e.g. start with "This built_option function.."
  5. 2f4c48c (Alias VCPKG_TARGET_TRIPLET to VCPKG_ARCH.) - Aside: does all this work with Visual studio 2019, as well as Ninja (cross checking). "build_option() function introduced in" - maybe say 'previous commit' rather than giving a sha1. (same for next commit messages).
  6. a21b05 (Alias VCPKG_ROOT env variable to VCPKG_DIR) - the reflow text part makes it tricky to see that no changes were made - tell the reader. sha1 thing..
  7. 319740 (Add nuget_install() function in cmake/NuGet.cmake) [Core New Stuff] - Should NuGet be protected with a USE_NUGET or equivalent? Mabe some don't want NuGet.
  8. d7901d (Use NuGet pkg Gettext.Tools for msgfmt on WIN32) - In the commit msg, note the double use (reuse) of find_program() i.e. that you hadn't forgotten to delete the later use..

When I tried to open the checked out cmake-improvements branch in my VS2019 (recent updates), it did not appear to run the CMakelists.txt actions and didn't start to compile - as if VS2019 hadn't even detected that git was a project.

Have you seen the same issue?

A bit of searching found some MS Visual Studio Articles, Partial Acivation and the Open Folder support. I.e. the Note: Starting in Visual Studio 2022 version 17.1 Preview 2, if your folder doesn't contain a root CMakeLists.txt you'll be.. .

I think it implies that the current use of a CMakelists.txt in a sub-directory will no longer work, and a root level CMakelists.txt will also be required - Have you seen the same issue?

Aside: does the t/test-lib.sh modification affect you when Visual Studio CMake runs - it leaves modifications? I'd suggested a workaround in Issue #3518 : "t/test-lib.sh: is modified when starting Visual Studio, with the open folder git mechanism", but hadn't had time to implement it.

@rkitover
Copy link
Author

Hi @PhilipOakley, thank you for the detailed response.

I should be able to take care of most of this over the next few days.

This is also great practice for me, I need to learn how to do things like this for more serious projects and your feedback is very helpful. I used to have better commit discipline.

I will respond to some things you brought up below.

Hi Rafael, The split generally looks good. I did have a few small comments, some of which are based on the main Git guidelines to keep consistency.

I fetched your changes via git fetch https://github.com/rkitover/git.git cmake-improvements:cmake-improvements and viewed them via gitk rather than using the GitHub interface.

1. the commit subject lines should omit the period. [no full stop](https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L155-L156)

2. Try not to reference previous commits by sha1, or sha1 alone. It's better to say 'previous commit', (cf 'subsequent commit') as the sha1 may change if there are edits, or they are submitted upstream as patches. The fallback is to use the `log --pretty=reference` to get the subject and date stamp as well. [commit-reference](https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L202-L223)

A few specifics

1. 5fc1f86 (Fix check for built vcpkg in vcpkg_install.bat.) - The 'some circumstances' - giving one example would help (just one is enough).

2. 0c5cb9 (Remove messages for uninitialized cmake vars) - those were from previous 'debug' sessions to make the achieved setup plain to the user. Dropping those is OK.

3. 6abf88 (Always call vcpkg_install.bat from cmake.) - Hmm. Is the message still suitable. Is a similar message already in vcpkg_install.bat? Does some other 'if' (not visible in the diff) already cover this? Worth clarifying in the message.

4. 580397(Add build_option() to cmake/Utilities.cmake.) - "This function performs does nearly the same thing as the builtin cmake" doesn't scan well, repetition? The description didn't flow that well - I have difficulty understanding what is being said in that sentence. Maybe the msg also needs "This function will be used in a subsequent commit".
   Don't assume the reader will remember the subject line while reading these longer commit messages, e.g. start with "This built_option function.."

5. 2f4c48c (Alias VCPKG_TARGET_TRIPLET to VCPKG_ARCH.) - Aside: does all this work with Visual studio 2019, as well as Ninja (cross checking). "build_option() function introduced in" - maybe say 'previous commit' rather than giving a sha1. (same for next commit messages).

6. a21b05 (Alias VCPKG_ROOT env variable to VCPKG_DIR) - the reflow text part makes it tricky to see that no changes were made - tell the reader. sha1 thing..

7. 319740 (Add nuget_install() function in cmake/NuGet.cmake)  [Core New Stuff] - Should NuGet be protected with a USE_NUGET or equivalent? Mabe some don't want NuGet.

I can do that, but I see no practical purpose for this other than someone having a philosophical objection to NuGet. The whole operation of downloading nuget.exe and installing this package takes a tiny fraction of a second. Also the Gettext.Tools package is a fallback, if the user already has a msgfmt utility in their PATH it will not be used.

8. d7901d (Use NuGet pkg Gettext.Tools for msgfmt on WIN32) - In the commit msg, note the double use (reuse) of `find_program()` i.e. that you hadn't forgotten to delete the later use..

When I tried to open the checked out cmake-improvements branch in my VS2019 (recent updates), it did not appear to run the CMakelists.txt actions and didn't start to compile - as if VS2019 hadn't even detected that git was a project.

Have you seen the same issue?

A bit of searching found some MS Visual Studio Articles, Partial Acivation and the Open Folder support. I.e. the Note: Starting in Visual Studio 2022 version 17.1 Preview 2, if your folder doesn't contain a root CMakeLists.txt you'll be.. .

I think it implies that the current use of a CMakelists.txt in a sub-directory will no longer work, and a root level CMakelists.txt will also be required - Have you seen the same issue?

I do not use the VS IDE much, I use PowerShell with the build tools mostly, but I did in fact try to play with this because having a root CMakeLists.txt is the idiomatic way to do this.

If I make a root CMakeLists.txt that uses include() or add_subdirectory() to load the file it causes various problems, but I can most likely make it work.

A symlink may work, with some code adjustments, this would require the developer to have developer mode enabled to get a real symlink in a clone, I think this is a reasonable assumption to make.

There are probably other ways to make opening the project in VS automatically work, like CMakeSettings.json. I can look at this.

Let me know what you would like to do, and if you have any other ideas, and I will address this in a separate PR.

Aside: does the t/test-lib.sh modification affect you when Visual Studio CMake runs - it leaves modifications? I'd suggested a workaround in Issue #3518 : "t/test-lib.sh: is modified when starting Visual Studio, with the open folder git mechanism", but hadn't had time to implement it.

I haven't even tried opening this in the IDE and/or running any of the tests, because my changes do not affect any of the tests, but I'll try to take a look.

@rkitover
Copy link
Author

1. 5fc1f86 (Fix check for built vcpkg in vcpkg_install.bat.) - The 'some circumstances' - giving one example would help (just one is enough).

I don't know actually, I think it failed for me once, but I'll just reword this to "improve."

@rkitover
Copy link
Author

5. 2f4c48c (Alias VCPKG_TARGET_TRIPLET to VCPKG_ARCH.) - Aside: does all this work with Visual studio 2019, as well as Ninja

I am using 2022 as of recently, before I used 2019, which is pretty much the same as far as these things go, and Ninja (on Windows the other generators are useless anyway.)

This doesn't affect anything anyway, it simply allows the dev to use -DVCPKG_TARGET_TRIPLET=... instead of -DVCPKG_ARCH=... if they so choose, VCPKG_ARCH will be set regardless.

@rkitover
Copy link
Author

3. 6abf88 (Always call vcpkg_install.bat from cmake.) - Hmm. Is the message still suitable. Is a similar message already in vcpkg_install.bat? Does some other 'if' (not visible in the diff) already cover this? Worth clarifying in the message.

If you mean the status message, I removed it and amended the "installing libraries" status message in the cmd script.

@rkitover
Copy link
Author

@PhilipOakley

I fixed all the titles according to the guidelines and proofread and reworded all the messages, and made a couple minor code changes.

  • See comment above about the vcpkg status message.
  • The other was clearing ENV{VCPKG_ROOT} in 099888b to make that commit independent.

Here are the new shas:

2ae6d985a6 build: use NuGet Gettext.Tools for msgfmt on WIN32
e68ce8a972 build: add nuget_install() in cmake/NuGet.cmake
b4238e47ee build: alias VCPKG_ROOT env variable to VCPKG_DIR
5830df7273 build: alias VCPKG_TARGET_TRIPLET to VCPKG_ARCH
1b27531077 build: add build_option() to cmake/Utilities.cmake
1e78440a7e build: always call vcpkg_install.bat from cmake
eff19a789f build: fix check for VCPKG_ARCH
257bee85c5 build: use VCPKG_DIR instead of constant
84535ba93a build: remove msgs for uninitialized cmake vars.
1fc9635af8 build: add workdir to CreateLinks.cmake call
c6f543a665 build: add vim modeline to CMakeLists.txt
59e02a0327 doc: correct NO_VCPKG to USE_VCPKG
099888bf7d build: support VCPKG_ROOT env in vcpkg_install.bat
a180f08de9 build: error/usage to stderr in vcpkg_install.bat
91c80f5889 build: reformat git check in vcpkg_install.bat
9f2ea77890 build: improve check for built vcpkg.exe
db74125d99 build: add -static triplets in vcpkg_install usage
08fc0814ba build: ignore build*/ for out-of-source builds

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

This was a really pleasant read!

My only objection is that we do not need to go back to NuGet to get the gettext package, we can use vcpkg for that (see the comments below).

Also, really, really crucial: please do not require msgfmt.exe if the user specifically asked for NO_GETTEXT.

Once those two things are addressed, I am happy to merge this PR!

contrib/buildsystems/CMakeLists.txt Show resolved Hide resolved
if(NOT MSGFMT_EXE)
if(WIN32)
include(NuGet)
nuget_install(Gettext.Tools)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether we can get a similar effect, albeit without having to use NuGet (which works very well for .NET projects but not well at all for C/C++ projects), if we used vcpkg for that? It does have the gettext package, after all.

Copy link
Author

Choose a reason for hiding this comment

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

NuGet is also used for build tools and utilities for various purposes.

I thought about using vcpkg for this, and you can build gettext[tools] to get msgfmt. However, gettext is a very long compile, and in this case we just want a utility not a link library, so I thought this would be a better way to go. I use this method in another project.

However, if you want to support one or the other or both I can do that.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not mix and match vcpkg and NuGet.

If you are really, really concerned, we can teach this Azure Pipeline to build gettext[tools] (or just gettext?) and offer it together with the other build artifacts.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW this Pipeline simply runs:

& compat/vcbuild/vcpkg_install.bat
if (!$?) { exit(1); }

& compat/vcbuild/vcpkg_install.bat arm64-windows
if (!$?) { exit(1); }

git init vcbuild-artifacts\compat\vcbuild\vcpkg
if (!$?) { exit(1); }
Copy-Item compat\vcbuild\vcpkg\vcpkg.exe vcbuild-artifacts\compat\vcbuild\vcpkg -Force
if (!$?) { exit(1); }
Copy-Item compat\vcbuild\vcpkg\installed vcbuild-artifacts\compat\vcbuild\vcpkg -Force -Recurse
if (!$?) { exit(1); }
Copy-Item compat\vcbuild\vcpkg\packages vcbuild-artifacts\compat\vcbuild\vcpkg -Force -Recurse
if (!$?) { exit(1); }
# Copy-Item compat\vcbuild\vcpkg\scripts vcbuild-artifacts\compat\vcbuild\vcpkg -Force -Recurse
# if (!$?) { exit(1); }

# Remove manual pages (they are not used, and only bloat the artifact)
Get-ChildItem vcbuild-artifacts\compat\vcbuild\vcpkg html -recurse -directory | ForEach-Object {Remove-Item $_.FullName -Recurse -Force}
if (!$?) { exit(1); }

Therefore we would probably need to handle NO_GETTEXT in vcpkg_install.bat, too: if it is set, do what we do right now. If it is not set, include gettext in the build.

Copy link
Author

Choose a reason for hiding this comment

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

My concern is making life less painful for contributors, when I say that gettext is a really long compile, I mean approximately half an hour on a typical system or more.

In my other vcpkg project, some of my contributors refused to use the VS support at all because of the long compile times.

But up to you.

This is a problem with vcpkg in general. Many people use Conan instead which has binary packages. I am planning to add Conan support to the other project I mentioned.

Speaking of compile times in a CI, vcpkg supports a new feature called binary caching: https://vcpkg.readthedocs.io/en/stable/users/binarycaching/

we would probably need to handle NO_GETTEXT in vcpkg_install.bat

For the purposes of this PR, I can just export this as an env var.

But this script needs some enhancements in general, like upgrading vcpkg itself and all dependencies.

Copy link
Author

Choose a reason for hiding this comment

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

I just verified that locales definitely DO NOT work for MSVC builds, even though translations are built now.

Copy link
Author

Choose a reason for hiding this comment

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

@PhilipOakley @dscho

I'm going to work on this possibly a few days more. It seems translations are not working on MSVC builds and there are a few other minor details to clean up.

Let me know your thoughts on the other points I brought up.

Copy link
Author

Choose a reason for hiding this comment

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

I meant to post that as a main comment, so include those as well.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know what GIT-BUILD-OPTIONS is used for

GIT-BUILD-OPTIONS is used e.g. in the test suite (e.g. to skip the translation-related tests).

I'm going to work on this possibly a few days more. It seems translations are not working on MSVC builds and there are a few other minor details to clean up.

Yes, let's focus on the common case where we simply won't need the translations.

If MSGFMT_EXE is set, then translations are built, nothing else controls this, the NO_GETTEXT variable has no effect on this.

That looks like a bug to me. I would really like to avoid even looking for MSGFMT_EXE if NO_GETTEXT is set to TRUE, and if NO_GETTEXT is not set and msgfmt cannot be found, I want NO_GETTEXT to be set to TRUE.

In any case, I want the translations not to be built when NO_GETTEXT is set to TRUE.

Copy link
Member

Choose a reason for hiding this comment

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

Could you please remove the need for NuGet, by respecting NO_GETTEXT to imply that MSGFMT_EXE is not needed?

contrib/buildsystems/CMakeLists.txt Show resolved Hide resolved
@rkitover
Copy link
Author

@dscho fixing the other two things right now, see my comment above re: NuGet.

If %VCPKG_ROOT% environment variable is set, check that the parent
directory exists and throw an error if it does not.

If %VCPKG_ROOT% is not set, set it to %cwd%\vcpkg, which was the
previous behavior.

Change all references to %cwd%\vcpkg to %VCPKG_ROOT%.

Change the documentation in the top comment to refer to %VCPKG_ROOT%.

This allows a developer to maintain his or her own vcpkg clone and not
have to unnecessarily repeat the very time-consuming process of
compiling the dependencies.

Set ENV{VCPKG_ROOT} to the VCPKG_DIR default in CMakeLists.txt, because
the cmake code does not support this yet, this is introduced in a
subsequent commit.

Signed-off-by: Rafael Kitover <[email protected]>
USE_VCPKG is the current variable used to disable vcpkg.

Signed-off-by: Rafael Kitover <[email protected]>
Add WORKING_DIRECTORY to the add_custom_command() call that launches
CreateLinks.cmake, which is a script that creates some hard links to
built git executables. This allows the script to not fail for build
directories other than the default.

Signed-off-by: Rafael Kitover <[email protected]>
Stop displaying the variables MSVC, CMAKE_CXX_COMPILER_ID,
CMAKE_GENERATOR_PLATFORM in the vcpkg setup section, as this section
runs before project() and they are not yet initialized.

Instead display them after the project() call, and use
CMAKE_C_COMPILER_ID instead of CMAKE_CXX_COMPILER_ID as this is a C
project.

Also change all variable log message() calls to message(STATUS ...)
calls.

Signed-off-by: Rafael Kitover <[email protected]>
Use VCPKG_DIR instead of relative path to the default vcpkg clone for
appending the vcpkg bin to PATH when writing GIT-BUILD-OPTIONS in cmake.

Signed-off-by: Rafael Kitover <[email protected]>
Use DEFINED instead of EXISTS (EXISTS is for checking if a filesystem
path exists, DEFINED is for variables.)

Also move the check to above the call to vcpkg_install.bat where it is
used from its previous location below.

Signed-off-by: Rafael Kitover <[email protected]>
Instead of checking if VCPKG_DIR exists first, always call
vcpkg_install.bat, which will create a new vcpkg clone as well as
perform any necessary actions on an existing clone, and will do nothing
if nothing needs to be done.

Remove the "Initializing vcpkg..." message from cmake and change the
"installing libraries" message from the cmd script to say that it may
take a while.

Signed-off-by: Rafael Kitover <[email protected]>
@rkitover
Copy link
Author

@dscho @PhilipOakley

Hi guys I finished some cleanup of all of this, but, I cannot , for the life of me, figure out how to use one of the generated .mo files for translation out of the build directory. It links gettext fine and builds the .mo files fine however.

Can I please have some hints about how to test this?

@rkitover
Copy link
Author

I used a hack to demonstrate this, but gettext is working in VS build now! See git status at the bottom in Russian in this screenshot:

screenshot in Russian

@dscho
Copy link
Member

dscho commented Mar 18, 2022

Nice!

I meant to reply that you can set GIT_TEXTDOMAINDIR to test translations (it's used here), but it seems you found a different, equally valid way.

endforeach()
endfunction()

# vim:set sw=8 ts=8 noet:
Copy link
Member

Choose a reason for hiding this comment

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

That's quite the addition for but a single user... Is there actually a need for this, apart from a mere preference for a documented option?

Copy link
Author

Choose a reason for hiding this comment

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

I did this to make aliasing options and env vars cleaner, it otherwise behaves almost the same way as the cmake built-in option(...).

Copy link
Author

Choose a reason for hiding this comment

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

Updated the docs and commit message.

Copy link
Member

Choose a reason for hiding this comment

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

It just adds a lot of code, and I am not sure that the benefit justifies that much.

Copy link
Author

Choose a reason for hiding this comment

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

I can remove it and do this manually if you like, but it would be a big ugly chain of if blocks, because the functions handles all aliases in order of precedence.

Copy link
Member

Choose a reason for hiding this comment

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

I honestly do not see enough value in that function to invest some 100 new lines of code in it, not even if I am "only" stuck with maintaining them.

Copy link
Member

Choose a reason for hiding this comment

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

In other words: I would prefer the flags to be specified via explicit command-line arguments to cmake, and not implicitly via some environment variables.

Copy link
Author

Choose a reason for hiding this comment

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

The whole point of all of this was to read VCPKG_ROOT from the environment implicitly. It's a documented variable:

https://vcpkg.readthedocs.io/en/latest/users/config-environment/

cmake uses other environment variables implicitly too, like CC/CXX.

Copy link
Author

Choose a reason for hiding this comment

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

@dscho I have a flight to catch tomorrow and need to pack, I will adjust everything as per your preferences over the next few days hopefully, and follow up on the manifest issue.

This comment was marked as off-topic.

Add the build_option() function which is similar to the cmake option()
command, but allows adding alias variables and alias environment
variables.

The option and all aliases are set to the first found value by listed
sequence in the option and all aliases, followed by the default if
provided, or 'OFF' if not provided, with cache variables having
precedence over normal variables.

In the case of environment variables or aliases, if the type is BOOL and
the found/default value is OFF/FALSE unset the environment variable.

Signed-off-by: Rafael Kitover <[email protected]>
Include the build_option() function introduced in b0d325d4bd (Add
build_option() to cmake/Utilities.cmake., 2022-02-10).

Initialize VCPKG_ARCH with an alias to VCPKG_TARGET_TRIPLET with
build_option().

Add a note about this to the cmake doc.

Signed-off-by: Rafael Kitover <[email protected]>
Use the build_option() function introduced in 3db0658 (build: add
build_option() to cmake/Utilities.cmake, 2022-02-10) to alias the
VCPKG_ROOT environment variable to VCPKG_DIR.

Support for VCPKG_ROOT was added to vcpkg_install.bat in 98c6a0d
(build: support VCPKG_ROOT env in vcpkg_install.bat, 2022-02-08).

Add a note about VCPKG_DIR and ENV{VCPKG_ROOT} to the cmake doc.

Signed-off-by: Rafael Kitover <[email protected]>
Check if NO_<dep> environment variable is defined to skip vcpkg deps,
this is needed to implement cmake support for NO_GETTEXT properly in a
subsequent commit.

Signed-off-by: Rafael Kitover <[email protected]>
Add gettext vcpkg dependency lib with the [tools] feature for the msgfmt
utility to build the translations.

Can be disabled by setting the NO_GETTEXT environment variable, support
for which was added in 2cd6269 (build: support NO_LIB env to disable
vcpkg deps, 2022-03-17).

Signed-off-by: Rafael Kitover <[email protected]>
Make NO_GETTEXT a build option aliased to ENV{NO_GETTEXT} to pass to
vcpkg_install.bat to skip building gettext.

Turn it off by default when vcpkg is in-use because it's a very long
compile.

Do not build translations if NO_GETTEXT is set.

Add a note about NO_GETTEXT to the docs at the top of CMakeLists.txt.

Signed-off-by: Rafael Kitover <[email protected]>
@rkitover
Copy link
Author

@dscho @PhilipOakley

I'd like to summarize what this PR contains currently and hopefully finish whatever is necessary to get it merged.

Motivations:

Improvements to vcpkg_install.bat:

  • Use %VCPKG_ROOT% from the environment instead of hardcoded vcpkg path, with the default being the previous value.
  • Check for existing vcpkg.exe under %VCPKG_ROOT% to detect a valid vcpkg tree, in which case just install the libraries, which will not be installed if they are already installed.
  • Add gettext[tools] to the list of vcpkg packages to install.
  • Support setting %NO_GETTEXT% or %NO_<DEP>% to skip gettext (the default in the cmake) or any other dep.

Improvements to CMakeLists.txt:

  • Fix support for NO_GETTEXT option, you wanted this @dscho. It's aliased to the environment value because this is used in the other build system, but I can remove that. It defaults to off because gettext is a very long build.
  • Alias VCPKG_DIR to ENV{VCPKG_ROOT}, this is the primary purpose of this PR.
  • Alias VCPKG_ARCH to VCPKG_TARGET_TRIPLET, I can remove this, I was also thinking off adding an ENV{VCPKG_DEFAULT_TRIPLET} alias, as described in the vcpkg support env vars page above.
  • Fix/improve diagnostic output.
  • Minor fix for the git-remote-http command alias, which was failing the build.

I can remove the build_option function if you insist, but I'd like to keep the ENV{VCPKG_ROOT} support because that was the main thing I wanted to do here.

Please let me know how you'd like to proceed.

@rkitover
Copy link
Author

@dscho Awaiting your feedback, see post above. I put a lot of work into this and would like to finish it, as I said I will make any changes you want.

Copy link

@Ricsimond8774 Ricsimond8774 left a comment

Choose a reason for hiding this comment

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

(invalid, please ignore)

endforeach()
endfunction()

# vim:set sw=8 ts=8 noet:

This comment was marked as off-topic.

@Ricsimond8774

This comment was marked as off-topic.

@dscho
Copy link
Member

dscho commented Jul 15, 2022

@dscho Awaiting your feedback, see post above. I put a lot of work into this and would like to finish it, as I said I will make any changes you want.

I have too much on my plate, and will be mostly offline this coming week, so this PR will have to wait at least for another week.

@rkitover
Copy link
Author

No problem, thanks!

@rkitover
Copy link
Author

@dscho Hi, any chance you could take a look at this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants