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

[tgbot-cpp] Update to v1.3 #23947

Merged
merged 2 commits into from
Apr 6, 2022
Merged

Conversation

ZeeWanderer
Copy link
Contributor

Updates tgbot-cpp port to version 1.3. Also updates manifest and portfile to match new maintainer guidelines.

Copy link
Contributor

@Thomas1664 Thomas1664 left a comment

Choose a reason for hiding this comment

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

Can you please add quotes around each path that starts with ${<CMAKE_VAR>} as well?

ports/tgbot-cpp/vcpkg.json Outdated Show resolved Hide resolved
Comment on lines 10 to 11
SOURCE_PATH ${SOURCE_PATH}
PREFER_NINJA
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SOURCE_PATH ${SOURCE_PATH}
PREFER_NINJA
SOURCE_PATH "${SOURCE_PATH}"

Copy link
Contributor Author

@ZeeWanderer ZeeWanderer Apr 2, 2022

Choose a reason for hiding this comment

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

I can do that but why? I have not seen this in maintainers guide and there is no reason to do that other than personal preference plus this format was inherited from previous port version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because they are variable expansions. If a path contains spaces, this would be a problem if you don't use quotes. See the CMake guide: https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/cmake-guidelines.md

Copy link
Contributor Author

@ZeeWanderer ZeeWanderer Apr 2, 2022

Choose a reason for hiding this comment

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

If maintainers guide does actually specify that then ok, but it won't actually cause any problems with variable passing in this case. In fact, quotes are redundant in a lot of places regardless of whitespaces. But i guess paranoidal wins.

cmake_minimum_required(VERSION 3.23)

set(TEST_VARIABLE "" CACHE FILEPATH "A file with whitespaces in path")

function(test_func)
    cmake_parse_arguments(PARSE_ARGV 0 "arg"
        ""
        "PATH_;TEST_"
        ""
    )
    message(STATUS ${arg_PATH_})
endfunction()

test_func(
    PATH_ ${TEST_VARIABLE}
)

Now choose a file with GUI That contains whitespaces in its path and see that it works correctly. Because of C family language habits people use quotes a lot when there is no need for them in CMake.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ZeeWanderer The PREFER_NINJA line has no effect on vcpkg_cmake_configure. Can you please remove it?

Copy link
Contributor Author

@ZeeWanderer ZeeWanderer Apr 2, 2022

Choose a reason for hiding this comment

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

Expanding on that

cmake_minimum_required(VERSION 3.23)

set(TEST_VARIABLE "" CACHE PATH "A folder with whitespaces")

function(test_func)
    cmake_parse_arguments(PARSE_ARGV 0 "arg"
        ""
        "PATH_;TEST_"
        ""
    )
    message(STATUS ${arg_PATH_})

    if(${arg_PATH_}/test STREQUAL "${arg_PATH_}/test")
        message(STATUS "as expected they are equal")
    else()
        message(STATUS "boo")
    endif()

    if(IS_ABSOLUTE ${arg_PATH_})
        message(STATUS "as expected path is absolute")
    else()
        message(STATUS "boo")
    endif()
endfunction()

test_func(
    PATH_ ${TEST_VARIABLE}
)

Would print as expected they are equal and as expected path is absolute even if you select a folder path with whitespaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ZeeWanderer I didn't write the CMake guide. Please consider opening a discussion if you want to discuss this topic further.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is specifically so that semicolons don't screw with anything; we don't want to have different rules based on whether something is a path or not.

Also, if() is weird - for example, with strequal, if(${blah} STREQUAL "${blah}") will always succeed unless the value of ${blah} is empty or ${blah} expands to a variable name. For example:

set(blah "")
if(${blah} STREQUAL "${blah}") # error, `STREQUAL ""` is not a valid parameter list for `if`
endif()
set(blah "foo")
if(${blah} STREQUAL "${blah}")
  message(STATUS "this will print")
endif()
set(foo bar)
if(${blah} STREQUAL "${blah}")
  message(STATUS [[this will not, since "bar" != "foo"]])
endif()

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 01d6f6ff1e5332b926099f0c23bda996940ad4e8 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/t-/tgbot-cpp.json b/versions/t-/tgbot-cpp.json
index a82de3e..030a0cd 100644
--- a/versions/t-/tgbot-cpp.json
+++ b/versions/t-/tgbot-cpp.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "30895efbb49898202c290f86b6423f41d2dc66d1",
+      "git-tree": "34cdb866854d5942a93c978724cbcdbf300aa2ad",
       "version": "1.3",
       "port-version": 0
     },

@Adela0814 Adela0814 added category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines labels Apr 6, 2022
@strega-nil-ms
Copy link
Contributor

Thanks!

@strega-nil-ms strega-nil-ms merged commit bafc596 into microsoft:master Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants