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

[libmem] add new port #41345

Merged
merged 193 commits into from
Nov 22, 2024
Merged

[libmem] add new port #41345

merged 193 commits into from
Nov 22, 2024

Conversation

luadebug
Copy link
Contributor

@luadebug luadebug commented Oct 5, 2024

https://github.com/rdbo/libmem
It should be DLL or LIB.

  • No vcpkg_from_git.
  • Use vcpkg ports as dependencies.
  • Let vcpkg control the flags.
  • Remove the corresponding include(), overwrite it with an empty string, or remove the CMAKE_GENERATOR lines.
  • The GENERATOR option should probably just be removed (which selects the Ninja generator).
  • PreLoad.cmake overwrite it with an empty string.
  • set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON) is removed & followed upstream by rdbo/libmem@04830fb as patch.
  • INTERFACE $<INSTALL_INTERFACE:include>is applied.
  • vcpkg_from_github uses REF ${VERSION} as param.
  • Port build time is simplified from 1.5h to 2m by using bundled LLVM module of Demangler.
  • Upstream`s CMakeLists.txt is being patched now, port goes to be official.
  • In addition, using imported targets mandates find_dependency in exported config.
  • Use vcpkg_find_acquire_program(PKGCONFIG), and pass it to CMake with "-DPKG_CONFIG_EXECUTABLE=${PKGCONFIG}".
  • Redundant Findcapstone.cmake was removed.
  • Upstream`s patch content CMakeLists.txt is trimmed.
  • Rely over pkgconfigs keystone.
  • Tests are persist disabled by default.
  • Fix .cmake.in & CMakeLists.txt to treat PkgConfigs keystone properly.

@luadebug
Copy link
Contributor Author

luadebug commented Oct 5, 2024

@microsoft-github-policy-service agree

@dg0yt
Copy link
Contributor

dg0yt commented Oct 5, 2024

This will need many changes.

  • No vcpkg_from_git
  • Use vcpkg ports as dependencies.
  • Let vcpkg control the flags.

Does it really have to use nmake? This limits the port to Windows hosts.

@luadebug luadebug marked this pull request as draft October 6, 2024 09:05
@Cheney-W Cheney-W added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Oct 8, 2024
@luadebug
Copy link
Contributor Author

luadebug commented Oct 13, 2024

This will need many changes.

* No `vcpkg_from_git`

* Use vcpkg ports as dependencies.

* Let vcpkg control the flags.

Does it really have to use nmake? This limits the port to Windows hosts.

  • No vcpkg_from_git - Done.
  • Use vcpkg ports as dependencies. - Done, but I wish I do not compile WHOLE LLVM dependency. I need only Demangle.
  • We need define LM_EXPORT in case we build static LIB for Windows OS.
    Not really, it should work for Linux OS as well, so I enforce NMake only in case of Windows OS build.
    Should something like this being used? Most likely LLVM would conflict about it.
if(VCPKG_TARGET_IS_WINDOWS AND VCPKG_CRT_LINKAGE STREQUAL "static")
    list(APPEND ADDITIONAL_OPTIONS
        -DITK_MSVC_STATIC_RUNTIME_LIBRARY=ON
        -DITK_MSVC_STATIC_CRT=ON
    )
endif()

Maybe I should enforce Unix Makefiles generator for x64-osx ?

@luadebug luadebug marked this pull request as ready for review October 13, 2024 23:25
@luadebug
Copy link
Contributor Author

I tried to fix vcpkg.json for LLVM but it did not work, it just fallback to default features of LLVM package.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 14, 2024

I tried to fix vcpkg.json for LLVM but it did not work, it just fallback to default features of LLVM package.

In a port manifest, default-features is just an enabler (but useful!). The actual control comes from the top-level install request, which is either the command line (classic mode) or top-level manifest (manifest mode).

@Cheney-W
Copy link
Contributor

I got below error when I test the usage of libmem:x64-windows-static:

CMake Error at E:/VS2022/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.29/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
1> [CMake]   Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)

@luadebug luadebug marked this pull request as draft November 12, 2024 11:06
@luadebug
Copy link
Contributor Author

We could go for Findkeystone.cmake.
Or either you could try to download msys64 and try again.

ports/libmem/portfile.cmake Outdated Show resolved Hide resolved
@dg0yt
Copy link
Contributor

dg0yt commented Nov 12, 2024

We could go for Findkeystone.cmake.

The module you had didn't solve the problem but just added a layer of indirection. (#41345 (comment)).
To avoid requiring pkgconfig downstream, don't use the imported target from pkg_check_modules but <prefix>_LINK_LIBRARIES.

I don't know why it didn't install pkgconfig during vcpkg_find_acquire_program(PKGCONFIG) call though.

It does, when building libmem. That's why building succeeds.

The problem occurs when using libmem (exported config).

@luadebug
Copy link
Contributor Author

luadebug commented Nov 13, 2024

We could go for Findkeystone.cmake.

The module you had didn't solve the problem but just added a layer of indirection. (#41345 (comment)).
To avoid requiring pkgconfig downstream, don't use the imported target from pkg_check_modules but <prefix>_LINK_LIBRARIES.

I don't know why it didn't install pkgconfig during vcpkg_find_acquire_program(PKGCONFIG) call though.

It does, when building libmem. That's why building succeeds.

The problem occurs when using libmem (exported config).

Can you please provide more information about further changes that should be done @dg0yt ?

Do we need to remain everything as is and change .cmake.in file like this

@PACKAGE_INIT@ 
  
include(CMakeFindDependencyMacro) 
find_dependency(capstone CONFIG REQUIRED)

include("${CMAKE_CURRENT_LIST_DIR}/libmem-target.cmake")

set(INSTALLED_ROOT "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}")

set(KEYSTONE_LIBRARY_DIRS "${INSTALLED_ROOT}/lib")

set(KEYSTONE_INCLUDE_DIRS "${INSTALLED_ROOT}/include")

find_path(KEYSTONE_INCLUDE_DIR 
    NAMES keystone.h
    PATHS ${KEYSTONE_INCLUDE_DIRS}
    PATH_SUFFIXES keystone
    REQUIRED
)

find_library(KEYSTONE_LIBRARY_DEBUG 
    NAMES keystoned
    PATHS ${KEYSTONE_LIBRARY_DIRS}
    PATH_SUFFIXES debug
    REQUIRED
)

find_library(KEYSTONE_LIBRARY_RELEASE 
    NAMES keystone
    PATHS ${KEYSTONE_LIBRARY_DIRS}
    REQUIRED
)

include(SelectLibraryConfigurations)
select_library_configurations(keystone)

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(
    keystone
    REQUIRED_VARS KEYSTONE_LIBRARY KEYSTONE_INCLUDE_DIR
)

target_link_libraries(libmem::libmem INTERFACE capstone::capstone ${KEYSTONE_LIBRARY})
#This line is redundant because we already have required dependencies
#check_required_components(libmem)

Or I need to copy paste keystone search mechanics into CMakeLists.txt as well & link against found keystone?

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

If you do the right thing in CMakeLists.txt, you need do nothing in the exported cmake config.

ports/libmem/0001-CMakeLists.patch Outdated Show resolved Hide resolved
ports/libmem/0001-CMakeLists.patch Outdated Show resolved Hide resolved
ports/libmem/libmem-config.cmake.in Outdated Show resolved Hide resolved
@luadebug
Copy link
Contributor Author

luadebug commented Nov 14, 2024

It's success over static builds but fails over dynamic builds due keystone linkage as for my side & as azure CI side in case of Windows OS.
@dg0yt there prob typo.

@luadebug luadebug marked this pull request as ready for review November 15, 2024 10:59
@luadebug luadebug requested a review from dg0yt November 15, 2024 11:00
@luadebug
Copy link
Contributor Author

luadebug commented Nov 15, 2024

Tested over x86-windows working.
@dg0yt feel free to review.

@Cheney-W
Copy link
Contributor

Usage test passed with x64-windows-static.

Cheney-W
Cheney-W previously approved these changes Nov 19, 2024
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Nov 19, 2024
@BillyONeal
Copy link
Member

This is kind of a lot of patching, have these patches been submitted upstream?

@luadebug
Copy link
Contributor Author

luadebug commented Nov 20, 2024

This is kind of a lot of patching, have these patches been submitted upstream?

rdbo/libmem@04830fb
I tend to create three patch files to follow upstream. 5.0.2 (latest available released tag for now) Itself does not contain api.h to rely over with libmem.h/libmem.hpp over it. Meanwhile upstream uses api.h & relies over it by libmem.h/libmem.hpp.

It took lot of time to make CMakeLists.txt to behave expected way for vcpkg (dependencies & .in files), honestly I do not find idea of mimic that being created by upstream useful, hence vcpkg is tend to avoid using something like add_custom_command, execute_process & so on.

@BillyONeal
Copy link
Member

Thanks!

@BillyONeal BillyONeal merged commit 199ea6e into microsoft:master Nov 22, 2024
17 checks passed
@luadebug
Copy link
Contributor Author

luadebug commented Nov 23, 2024

@BillyONeal
Thanks everyone for help, I still think its best for vcpkg.json to store this, during CMakeLists.txt content.

"supports": "(windows | linux | freebsd) & (x86 | x64)"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants