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

[tinyfiledialogs] 3.6.3 -> 3.8.8 #17343

Merged
merged 20 commits into from
May 18, 2021

Conversation

SamuelMarks
Copy link
Contributor

@SamuelMarks SamuelMarks commented Apr 18, 2021

Describe the pull request
https://sourceforge.net/p/tinyfiledialogs/code/ci/ab6f4f916aaa95d05247ffa66a30867e7f55e875/tree/

He didn't release a new zip so gotta change from vcpkg_from_sourceforge [which lists this package as an example] with vcpkg_download_distfile… not sure if there's a better way. Maybe it needs to be actually cloned rather than a snapshot downloaded? - Or maybe it needs to follow redirects? (like curl's -L flag).

EDIT: Switched to vcpkg_from_git; which seems to make the whole thing work.

  • What does your PR fix?

Upgrades the version

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    <all / linux, windows, ...>, <Yes/No>

  • Does your PR follow the maintainer guide?

    Your answer

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/


WiP, test with:

CMakeLists.txt

cmake_minimum_required(VERSION 3.4)
cmake_policy(SET CMP0048 NEW)
project(pp VERSION 0.0.0 LANGUAGES C)

file(DOWNLOAD "https://sourceforge.net/p/tinyfiledialogs/code/ci/4b65621952f2cf1045837a6380aa912b028daa47/tree/hello.c?format=raw" "src/main.c"
     EXPECTED_HASH SHA256=72502b4b5e93bf262b1ff148de028d12a187c1fda45de89212a3f896e2be1827)

set(src "${CMAKE_CURRENT_BINARY_DIR}/src/main.c")

find_package(tinyfiledialogs CONFIG REQUIRED)

add_executable("${PROJECT_NAME}" "${src}")

target_link_libraries("${PROJECT_NAME}" PRIVATE tinyfiledialogs::tinyfiledialogs)

EDIT: I was able to get it to work like so, but the help text doesn't tell the user to do this:

cmake_minimum_required(VERSION 3.4)
cmake_policy(SET CMP0048 NEW)
project(pp VERSION 0.0.0 LANGUAGES C)

file(DOWNLOAD "https://sourceforge.net/p/tinyfiledialogs/code/ci/4b65621952f2cf1045837a6380aa912b028daa47/tree/hello.c?format=raw" "src/main.c"
     EXPECTED_HASH SHA256=72502b4b5e93bf262b1ff148de028d12a187c1fda45de89212a3f896e2be1827)

set(src "${CMAKE_CURRENT_BINARY_DIR}/src/main.c")

find_package(tinyfiledialogs CONFIG REQUIRED)
find_path(TINYFILEDIALOGS_INCLUDE_DIRS "tinyfiledialogs.h")

add_executable("${PROJECT_NAME}" "${src}")
target_include_directories("${PROJECT_NAME}" PRIVATE "${TINYFILEDIALOGS_INCLUDE_DIRS}")
target_link_libraries("${PROJECT_NAME}" PRIVATE tinyfiledialogs::tinyfiledialogs)

@SamuelMarks SamuelMarks marked this pull request as ready for review April 19, 2021 00:08
@JonLiu1993 JonLiu1993 self-assigned this Apr 19, 2021
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Apr 19, 2021
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Apr 19, 2021
@JonLiu1993
Copy link
Member

@SamuelMarks , thaks for your contribution

@SamuelMarks
Copy link
Contributor Author

SamuelMarks commented Apr 19, 2021

@JonLiu1993 No worries.

On a related note I tried to make this header-only, by removing the CMakeLists.txt and changing portfile.cmake to:

vcpkg_fail_port_install(ON_TARGET "uwp")

set(TINYFILEDIALOGS_VERSION "3.8.7")
set(SOURCE_PATH "${CURRENT_BUILDTREES_DIR}/src/tinyfiledialogs-${TINYFILEDIALOGS_VERSION}")

vcpkg_from_git(
    OUT_SOURCE_PATH "${SOURCE_PATH}"
    URL "https://git.code.sf.net/p/tinyfiledialogs/code"
    REF "ab6f4f916aaa95d05247ffa66a30867e7f55e875"
)

# tinyfiledialogs.amalgamation.h
file(READ "${${SOURCE_PATH}}/tinyfiledialogs.c" _c)
file(READ "${${SOURCE_PATH}}/tinyfiledialogs.h" _h)
string(FIND "${_c}" "char tinyfd_version" _c_source_idx)
string(SUBSTRING "${_c}" 0 "${_c_source_idx}" _c_source_head)
string(SUBSTRING "${_c}" "${_c_source_idx}" -1 _c_source)
file(WRITE "${SOURCE_PATH}/tinyfiledialogs.h" "${_h}" "${_c_source_head}" "${_c_source}")

file(INSTALL "${SOURCE_PATH}/tinyfiledialogs.h" DESTINATION "${CURRENT_PACKAGES_DIR}/include")
file(READ "${CURRENT_PACKAGES_DIR}/include/tinyfiledialogs.h" _contents)
string(SUBSTRING "${_contents}" 0 1024 _contents)
file(WRITE "${CURRENT_PACKAGES_DIR}/share/${PORT}/copyright" "${_contents}")

vcpkg_copy_pdbs()

But currently getting a bunch too many error: redefinition of issues. Any suggestions?

@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Apr 19, 2021
@JackBoosY
Copy link
Contributor

@SamuelMarks What problem did you encounter?

@SamuelMarks
Copy link
Contributor Author

So the code in this PR works. It just requires a library linkage. I'm trying to get it working without that extra step by combining the .c and .h file into one, but that gives me these errors:

tinyfiledialogs.h:621:6:  error: redefinition of 'tfd_replaceSubStr'
tinyfiledialogs.h:598:13: error: redefinition of 'RGB2Hex'
tinyfiledialogs.h:574:13: error: redefinition of 'Hex2RGB'
tinyfiledialogs.h:561:13: error: redefinition of 'ensureFinalSlash'
tinyfiledialogs.h:531:15: error: redefinition of 'getLastName'
tinyfiledialogs.h:501:15: error: redefinition of 'getPathWithoutFinalSlash'

Is there a trick to this? - Like maybe walking the symbols ensuring the prototypes appear at the top and the implementations at the bottom, and no duplication?

@SamuelMarks
Copy link
Contributor Author

@JackBoosY As it stands however, this can definitely be merged into master.

@JackBoosY
Copy link
Contributor

@JonLiu1993 No worries.

On a related note I tried to make this header-only, by removing the CMakeLists.txt and changing portfile.cmake to:

vcpkg_fail_port_install(ON_TARGET "uwp")

set(TINYFILEDIALOGS_VERSION "3.8.7")
set(SOURCE_PATH "${CURRENT_BUILDTREES_DIR}/src/tinyfiledialogs-${TINYFILEDIALOGS_VERSION}")

vcpkg_from_git(
    OUT_SOURCE_PATH "${SOURCE_PATH}"
    URL "https://git.code.sf.net/p/tinyfiledialogs/code"
    REF "ab6f4f916aaa95d05247ffa66a30867e7f55e875"
)

# tinyfiledialogs.amalgamation.h
file(READ "${${SOURCE_PATH}}/tinyfiledialogs.c" _c)
file(READ "${${SOURCE_PATH}}/tinyfiledialogs.h" _h)
string(FIND "${_c}" "char tinyfd_version" _c_source_idx)
string(SUBSTRING "${_c}" 0 "${_c_source_idx}" _c_source_head)
string(SUBSTRING "${_c}" "${_c_source_idx}" -1 _c_source)
file(WRITE "${SOURCE_PATH}/tinyfiledialogs.h" "${_h}" "${_c_source_head}" "${_c_source}")

file(INSTALL "${SOURCE_PATH}/tinyfiledialogs.h" DESTINATION "${CURRENT_PACKAGES_DIR}/include")
file(READ "${CURRENT_PACKAGES_DIR}/include/tinyfiledialogs.h" _contents)
string(SUBSTRING "${_contents}" 0 1024 _contents)
file(WRITE "${CURRENT_PACKAGES_DIR}/share/${PORT}/copyright" "${_contents}")

vcpkg_copy_pdbs()

But currently getting a bunch too many error: redefinition of issues. Any suggestions?

I want to know why you need to do this, can you explain more?

@SamuelMarks
Copy link
Contributor Author

There's no real reason for it. I just think that seeing as there are no external dependencies it would be nice if this library were header-only. That way it's simpler to manage its dependency.

Also vcpkg isn't telling the user to include the find_path and target_include_directories lines.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Apr 20, 2021
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

ports/tinyfiledialogs/portfile.cmake Outdated Show resolved Hide resolved
ports/tinyfiledialogs/portfile.cmake Outdated Show resolved Hide resolved
ports/tinyfiledialogs/portfile.cmake Outdated Show resolved Hide resolved
ports/tinyfiledialogs/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY JackBoosY added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Apr 24, 2021
@BillyONeal
Copy link
Member

Please merge with master before requesting a build again to get the current MacOS build fleet

versions/t-/tinyfiledialogs.json Outdated Show resolved Hide resolved
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels May 3, 2021
ports/tinyfiledialogs/portfile.cmake Outdated Show resolved Hide resolved
ports/tinyfiledialogs/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label May 6, 2021
@strega-nil-ms
Copy link
Contributor

@SamuelMarks I can't push to your branch, so would you mind making the changes I suggested? I know they're unrelated to your PR but might as well while you're making changes :)

@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

versions/t-/tinyfiledialogs.json Outdated Show resolved Hide resolved
@JonLiu1993 JonLiu1993 linked an issue May 14, 2021 that may be closed by this pull request
@xarthurx
Copy link
Contributor

For 3.8.8, the author did provide a new zip. So perhaps using the original workflow to directly update to the newest would be possible?

@JonLiu1993
Copy link
Member

For 3.8.8, the author did provide a new zip. So perhaps using the original workflow to directly update to the newest would be possible?

Good idea, @SamuelMarks, can you try it?

@SamuelMarks
Copy link
Contributor Author

@JonLiu1993 Hmm, I'm not sure if that will work. How do I pin it to a certain version?

There's no version number on the zip archive, and I can't see a way—correct me if you find one—for specifying tinyfiledialogs-current.zip at said hash.

@SamuelMarks SamuelMarks changed the title [tinyfiledialogs] 3.6.3 -> 3.8.7 [tinyfiledialogs] 3.6.3 -> 3.8.8 May 14, 2021
@JonLiu1993 JonLiu1993 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels May 15, 2021
@dan-shaw dan-shaw merged commit ebdbcd2 into microsoft:master May 18, 2021
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.

[tinyfiledialogs] update to 3.8.8
8 participants