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

[arpack-ng] Add new port #29248

Merged
merged 3 commits into from
Feb 27, 2023
Merged

Conversation

FabienPean
Copy link
Contributor

@FabienPean FabienPean commented Jan 29, 2023

  • Changes comply with the maintainer guide
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See docs/examples/adding-an-explicit-usage.md for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

The PR is a draft and does not contain correct versioning scheme at the moment. I want to see if the results of the CI here and then request a new release upstream if no upstream changes needed, updated here afterwards when finalizing the PR. Only core package is integrated at the moment, all options are disabled.

This is a Fortran package with plenty of quirks (msys/mingw related on Windows especially), so there are some things I must have missed, feedback appreciated !

Minimal compiling example: arpack_mwe.zip

github-actions[bot]
github-actions bot previously approved these changes Jan 29, 2023
@FabienPean
Copy link
Contributor Author

FabienPean commented Jan 29, 2023

Failure on x64_uwp https://github.com/microsoft/vcpkg/pull/29248/checks?check_run_id=10957599574

-- Found BLAS: D:/installed/x64-uwp/lib/openblas.lib  
-- Using VCPKG FindLAPACK from package 'clapack'
-- LAPACK_DLL_DIR: D:/installed/x64-uwp/include/../bin;D:/installed/x64-uwp/include/../bin
-- Found LAPACK: optimized;D:/installed/x64-uwp/lib/lapack.lib;optimized;D:/installed/x64-uwp/lib/libf2c.lib;optimized;D:/installed/x64-uwp/lib/openblas.lib;debug;D:/installed/x64-uwp/debug/lib/lapackd.lib;debug;D:/installed/x64-uwp/debug/lib/libf2cd.lib;debug;D:/installed/x64-uwp/debug/lib/openblas.lib  
CMake Error at CMakeLists.txt:300 (set_target_properties):
  Property INTERFACE_LINK_LIBRARIES may not contain link-type keyword
  "optimized".  The INTERFACE_LINK_LIBRARIES property may contain
  configuration-sensitive generator-expressions which may be used to specify
  per-configuration rules.

it relies on clapack which does not seem to offer CMake built-in targets LAPACK::LAPACK out of find_package(LAPACK REQUIRED) though according to https://github.com/microsoft/vcpkg/blob/master/ports/clapack/FindLAPACK.cmake it is actually creating such target

This leads to the trigger of this part of the upstream CMakeLists

    find_package(LAPACK REQUIRED)

    # LAPACK::LAPACK target was already created at this point by FindLAPACK.cmake if cmake version >= 3.18
    if (NOT TARGET LAPACK::LAPACK) # Create target "at hand" to ensure compatibility if cmake version < 3.18
        add_library(LAPACK::LAPACK INTERFACE IMPORTED)
        set_target_properties(LAPACK::LAPACK PROPERTIES INTERFACE_LINK_LIBRARIES "${LAPACK_LIBRARIES}")
    endif()

Is it something to fix upstream or vcpkg related ? Or simply disabling support for uwp

On another note, I am quite surprised to see the package pass on arm

@FabienPean FabienPean marked this pull request as ready for review January 29, 2023 22:21
@FabienPean FabienPean marked this pull request as draft January 29, 2023 22:32
@LilyWangLL LilyWangLL self-assigned this Jan 30, 2023
@LilyWangLL LilyWangLL added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jan 30, 2023
@BillyONeal
Copy link
Member

I manually checked that the SHA in here is not affected by #29288

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!

All manifest files must be formatted

./vcpkg format-manifest ports/*/vcpkg.json

Diff
diff --git a/ports/arpack-ng/vcpkg.json b/ports/arpack-ng/vcpkg.json
index 2302506..b56e399 100644
--- a/ports/arpack-ng/vcpkg.json
+++ b/ports/arpack-ng/vcpkg.json
@@ -4,7 +4,7 @@
   "description": "ARPACK-NG is a collection of Fortran77 subroutines designed to solve large scale eigenvalue problems.",
   "homepage": "https://github.com/opencollab/arpack-ng",
   "license": "BSD-3-Clause",
-  "supports":"!uwp",
+  "supports": "!uwp",
   "dependencies": [
     "blas",
     "lapack",
PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for arpack-ng have changed but the version was not updated
version: 2023-01-29
old SHA: 8818ebc8d15989b5ac8f19a2a2a2a8e065f95812
new SHA: 0f862bc522022971d4c294d9df3f44e3ba8b7e39
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

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!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for arpack-ng have changed but the version was not updated
version: 2023-01-29
old SHA: 8818ebc8d15989b5ac8f19a2a2a2a8e065f95812
new SHA: 0f862bc522022971d4c294d9df3f44e3ba8b7e39
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@FabienPean FabienPean marked this pull request as ready for review February 1, 2023 21:12
@FabienPean
Copy link
Contributor Author

FabienPean commented Feb 1, 2023

Ready for review I guess. Here some potential points of discussion:

  • uwp is skipped for now because of the problem mentioned in previous comment.
  • I am not sure how to set the ci.baseline.txt file here or if it is necessary besides the supports clause in the manifest
  • arm is passing on the CI but I am not convinced it is really supported (ci does not seem to build anything)
  • The port installs several include files with common names in its own folder but they are still used as in #include<arpack.hpp> not sure if it conflicts with this rule. Though the package principal usage only requires name-specific includes (prefix arpack...)
  • Does it require further upstream changes before an initial accept of the port ? If not, then I would request a new release to use the correct version scheme for the package.

@LilyWangLL
Copy link
Contributor

I installed arpack-ng:x64-windows locally, there is a libarpack.dll.a in lib folder, is it correct?
image

github-actions[bot]
github-actions bot previously approved these changes Feb 2, 2023
@FabienPean
Copy link
Contributor Author

FabienPean commented Feb 2, 2023

Yes, the package is built through msys2 and mingw. It is similar in spirit to LAPACK

Arpack-ng is a fork of arpack, the original package being unavailable/unmaintained. Hence the library name. More info on wiki and readme on their repo

Due to stalled upstream development, ARPAСK has been forked into ARPACK-NG, as a form of a collaborative effort of the various groups that rely on ARPACK

LilyWangLL
LilyWangLL previously approved these changes Feb 3, 2023
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Feb 3, 2023
BillyONeal
BillyONeal previously approved these changes Feb 3, 2023
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

This LGTM as soon as upstream does the release to get versioning to match.

ports/arpack-ng/portfile.cmake Show resolved Hide resolved
@BillyONeal BillyONeal added depends:upstream-changes Waiting on a change to the upstream project and removed info:reviewed Pull Request changes follow basic guidelines labels Feb 3, 2023
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!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for arpack-ng have changed but the version was not updated
version: 2023-01-29
old SHA: 0f862bc522022971d4c294d9df3f44e3ba8b7e39
new SHA: 5f8e3a22893654507fc8f8a29c7dd4e3a81df4b5
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@FabienPean
Copy link
Contributor Author

Sorry for potential notification overload, I thought it was possible to re-request review from both. Also, looks like the bot is KO

@FabienPean FabienPean requested review from github-actions[bot] and removed request for LilyWangLL February 23, 2023 07:39
@LilyWangLL LilyWangLL added info:reviewed Pull Request changes follow basic guidelines and removed depends:upstream-changes Waiting on a change to the upstream project labels Feb 27, 2023
@JavierMatosD
Copy link
Contributor

Thanks!

@JavierMatosD JavierMatosD merged commit 5626cee into microsoft:master Feb 27, 2023
@FabienPean FabienPean deleted the port/arpack branch February 27, 2023 22:36
@JackBoosY
Copy link
Contributor

JackBoosY commented Feb 28, 2023

-- Performing post-build validation
warning: Detected outdated dynamic CRT in the following files:
    D:\packages\arpack-ng_x86-windows\debug\bin\libarpack.dll:msvcrt.dll
    D:\packages\arpack-ng_x86-windows\bin\libarpack.dll:msvcrt.dll
To inspect the dll files, use:
    dumpbin.exe /dependents mylibfile.dll
error: Found 1 post-build check problem(s). To submit these ports to curated catalogs, please first correct the portfile: C:\a\1\s\ports\arpack-ng\portfile.cmake
error: building arpack-ng:x86-windows failed with: POST_BUILD_CHECKS_FAILED

See https://dev.azure.com/vcpkg/public/_build/results?buildId=86045&view=results

I don't have a Windows machine here so I can't help you guys to fix that.
cc @Cheney-W

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.

5 participants