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

[spglib] Add a new port #39493

Merged
merged 1 commit into from
Jul 8, 2024
Merged

[spglib] Add a new port #39493

merged 1 commit into from
Jul 8, 2024

Conversation

LecrisUT
Copy link
Contributor

@LecrisUT LecrisUT commented Jun 24, 2024

  • 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 adding-usage 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.

This is the first time researching vcpkg packaging, I would appreciate any feedback and suggestions that would need to be upstreamed. I do not have a vcpkg setup so I am relying a bit on the CI system. If there is some setup for packagers either for CI/CD in upstream or containerfiles setup, it would help immensely.

@LecrisUT
Copy link
Contributor Author

Regarding "Contribution License Agreement" I am unclear on what is being targeted there? Is it solely the commits on this repo, or are they referring to the package's source code being packaged?

@LecrisUT LecrisUT force-pushed the spglib branch 3 times, most recently from 44f0246 to da4de0e Compare June 24, 2024 17:59
@BillyONeal BillyONeal marked this pull request as draft June 25, 2024 01:53
@BillyONeal
Copy link
Member

Regarding "Contribution License Agreement" I am unclear on what is being targeted there? Is it solely the commits on this repo, or are they referring to the package's source code being packaged?

I am not a lawyer but my understanding is that it applies to your contributions to the repo.

Please feel free to click 'ready for review' if you accept the CLA and fix build failures.

@MonicaLiu0311 MonicaLiu0311 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jun 25, 2024
@LecrisUT
Copy link
Contributor Author

@microsoft-github-policy-service agree

@LecrisUT LecrisUT force-pushed the spglib branch 3 times, most recently from 64ef927 to 5b6013d Compare June 25, 2024 08:56
ports/spglib/portfile.cmake Show resolved Hide resolved
ports/spglib/portfile.cmake Outdated Show resolved Hide resolved
@LecrisUT LecrisUT marked this pull request as ready for review June 25, 2024 09:09
@LecrisUT LecrisUT force-pushed the spglib branch 3 times, most recently from e92dcee to eae8651 Compare June 25, 2024 10:22
@MonicaLiu0311 MonicaLiu0311 marked this pull request as draft June 27, 2024 07:34
@LecrisUT LecrisUT marked this pull request as ready for review July 1, 2024 08:08
@MonicaLiu0311
Copy link
Contributor

usage:

spglib provides CMake targets:

  # this is heuristically generated, and may not be correct
  find_package(Spglib CONFIG REQUIRED)
  target_link_libraries(main PRIVATE Spglib::symspg Spglib::fortran)

spglib provides pkg-config modules:

  # The spglib library
  spglib

When testing usage, the following error occurs:

1> [CMake] CMake Error at test/CMakeLists.txt:7 (target_link_libraries):
1> [CMake]   Target "main" links to:
1> [CMake] 
1> [CMake]     Spglib::fortran
1> [CMake] 
1> [CMake]   but the target was not found.  Possible reasons include:
1> [CMake] 
1> [CMake]     * There is a typo in the target name.
1> [CMake]     * A find_package call is missing for an IMPORTED target.
1> [CMake]     * An ALIAS target is missing.
test.cpp
#include <iostream>
#include "spglib.h"

using namespace std;

int main()
{
cout << "Hello CMake." << endl;
return 0;
}

CMakeLists.txt
cmake_minimum_required (VERSION 3.8)

set(CMAKE_TOOLCHAIN_FILE "G:/spg/vcpkg/scripts/buildsystems/vcpkg.cmake")

project ("test")

add_library (main "test.cpp")

find_package(Spglib CONFIG REQUIRED)
target_link_libraries(main PRIVATE Spglib::symspg Spglib::fortran)

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Jul 1, 2024

usage:

Modern CMake design is to use FetchContent and rely on FIND_PACKAGE_ARGS. spglib was converted to allow this modern workflow. What do you think about advertising that?

Regarding fortran. This is disabled, you should only link to Splib::symspg. Fortran is rather tricky to package because it is ABI incompatible, so it should be installed in compiler specific folders if library files are to be installed for the user. Does vcpkg distribute binaries, or in the process of importing a vcpkg package, is the install step used? If so, how does vcpkg deal with changing compilers (actually this is an issue for C++ as well) and/or caching.

@LecrisUT LecrisUT force-pushed the spglib branch 3 times, most recently from 32c55b3 to d85e36a Compare July 1, 2024 13:11
ports/spglib/usage Outdated Show resolved Hide resolved
ports/spglib/portfile.cmake Outdated Show resolved Hide resolved
ports/spglib/portfile.cmake Outdated Show resolved Hide resolved
ports/spglib/usage Outdated Show resolved Hide resolved
@MonicaLiu0311
Copy link
Contributor

The usage test passed on x64-windows (header files found):

spglib provides CMake targets:

    find_package(Spglib CONFIG REQUIRED)
    target_link_libraries(main PRIVATE Spglib::symspg)

    # spglib is also compatible with modern CMake workflows
    include(FetchContent)
    FetchContent_Declare(Spglib
            GIT_REPOSITORY https://github.com/spglib/spglib
            GIT_TAG develop
    )
    FetchContent_MakeAvailable(Spglib ...)
    target_link_libraries(main PRIVATE Spglib::symspg)

spglib provides pkg-config modules:

    # The spglib library
    spglib

MonicaLiu0311
MonicaLiu0311 previously approved these changes Jul 4, 2024
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Jul 4, 2024
@dg0yt
Copy link
Contributor

dg0yt commented Jul 4, 2024

    find_package(Spglib CONFIG REQUIRED)
    target_link_libraries(main PRIVATE Spglib::symspg)

    # spglib is also compatible with modern CMake workflows
    include(FetchContent)
    FetchContent_Declare(Spglib
            GIT_REPOSITORY https://github.com/spglib/spglib
            GIT_TAG develop
    )
    FetchContent_MakeAvailable(Spglib ...)
    target_link_libraries(main PRIVATE Spglib::symspg)

Did you copy the whole block literally to CMakeLists.txt?

@MonicaLiu0311
Copy link
Contributor

Did you copy the whole block literally to CMakeLists.txt?

Yes, I tested the following two ways separately:

    find_package(Spglib CONFIG REQUIRED)
    target_link_libraries(main PRIVATE Spglib::symspg)
    # spglib is also compatible with modern CMake workflows
    include(FetchContent)
    FetchContent_Declare(Spglib
            GIT_REPOSITORY https://github.com/spglib/spglib
            GIT_TAG develop
    )
    FetchContent_MakeAvailable(Spglib)
    target_link_libraries(main PRIVATE Spglib::symspg)

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Jul 5, 2024

Should I separate the blocks a bit there, maybe like:

spglib provides CMake targets:

    find_package(Spglib CONFIG REQUIRED)
    target_link_libraries(main PRIVATE Spglib::symspg)

spglib is also compatible with modern CMake workflows

    include(FetchContent)
    FetchContent_Declare(Spglib
            GIT_REPOSITORY https://github.com/spglib/spglib
            GIT_TAG develop
            FIND_PACKAGE_ARGS CONFIG
    )
    FetchContent_MakeAvailable(Spglib ...)
    target_link_libraries(main PRIVATE Spglib::symspg)

I believe this point is valuable to document because not all projects are FetchContent compatible, and this is a recommended workflow to migrate towards. But if this continues to be a controversial issue, I will concede on the topic.

@dg0yt
Copy link
Contributor

dg0yt commented Jul 5, 2024

Your FetchContent implementation may be excellent, but that approach really doesn't integrate well with vcpkg's effort to control version, configuration and ABI. Simply remove the section which is unrelated to the vcpgk port.

Signed-off-by: Cristian Le <[email protected]>
@LecrisUT
Copy link
Contributor Author

LecrisUT commented Jul 5, 2024

Your FetchContent implementation may be excellent, but that approach really doesn't integrate well with vcpkg's effort to control version, configuration and ABI.

What I am trying to say is that it does integrate. I encourage you to try it out, particularly see the instructions here and look at the build directory and the presence/absence of _deps folder. Also, follow this and this upstream issues about the plans to further migrate these interfaces.

@MonicaLiu0311 MonicaLiu0311 removed the info:reviewed Pull Request changes follow basic guidelines label Jul 5, 2024
@dg0yt
Copy link
Contributor

dg0yt commented Jul 5, 2024

it does integrate

It integrate well with CMake but not with vcpkg IMO.

vcpkg makes an installation plan for installing dependencies for the top-level manifest (or command line), considering triplets, features, registries, overlay ports, version constraints. This determines which "spglib" to use. If the user project chooses, or falls back to, FetchContent, it is likely to result in another version and configuration than the one used by ports, and other than defined in the top-level manifest. The result may fail at build time or at runtime, in obvious or subtle ways.

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Jul 5, 2024

If the user project chooses, or falls back to, FetchContent, it is likely to result in another version and configuration than the one used by ports, and other than defined in the top-level manifest.

Setting CMAKE_REQUIRE_FIND_PACKAGE_Spglib would guarantee only find_package is considered. And dependency provider is the CMake recommended approach to give vcpkg full control.

@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Jul 8, 2024
@JavierMatosD JavierMatosD merged commit 47bf3d1 into microsoft:master Jul 8, 2024
17 checks passed
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