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.

Sign up for GitHub

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

Jump to bottom

[samurai] Add new port #31051

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

[samurai] Add new port #31051

wants to merge 9 commits into from
+104 −0

Conversation

gouarin
Copy link
Contributor

@gouarin gouarin commented Apr 22, 2023 •
edited
Loading

  • 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.
@FrankXie05 FrankXie05 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Apr 23, 2023
FrankXie05
FrankXie05 requested changes Apr 23, 2023
View reviewed changes
ports/samurai/portfile.cmake Outdated Show resolved Hide resolved
ports/samurai/portfile.cmake Outdated Show resolved Hide resolved
ports/samurai/portfile.cmake Outdated Show resolved Hide resolved
@FrankXie05 FrankXie05 changed the title [new port] samurai Apr 23, 2023
FrankXie05
FrankXie05 requested changes Apr 23, 2023
View reviewed changes
ports/samurai/portfile.cmake Outdated Show resolved Hide resolved
@gouarin
Copy link
Contributor Author

gouarin commented Apr 23, 2023

@FrankXie05 I don't understand why the REMOVE_RECURSEfailed now...
@FrankXie05
Copy link
Contributor

If you are header-only, the debug file should not exist. :)
@gouarin
Copy link
Contributor Author

gouarin commented Apr 23, 2023

@FrankXie05 I don't understand why the REMOVE_RECURSEfailed now...

I finally fixed this issue.
@gouarin
Copy link
Contributor Author

gouarin commented Apr 23, 2023

Now linux failed but I didn't change anything. It said that the configure_file defined here https://github.com/hpc-maths/samurai/blob/master/CMakeLists.txt#L13 doesn't exist. But it's not the case...
Is it possible to re-run the CI ?
@FrankXie05
Copy link
Contributor

/azp run
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).
FrankXie05
FrankXie05 previously approved these changes Apr 23, 2023
View reviewed changes
@FrankXie05
Copy link
Contributor

FrankXie05 commented Apr 23, 2023 •
edited
Loading

@gouarin #31051 (comment) Good suggestion. :)
dg0yt
dg0yt suggested changes Apr 23, 2023
View reviewed changes
ports/samurai/0002-fix-configure-file.patch Outdated
Comment on lines 11 to 17
configure_file(
- ${CMAKE_CURRENT_SOURCE_DIR}/version.hpp.in
- ${CMAKE_CURRENT_SOURCE_DIR}/include/samurai/version.hpp
+ "${CMAKE_CURRENT_SOURCE_DIR}/version.hpp.in"
+ "${CMAKE_CURRENT_SOURCE_DIR}/include/samurai/version.hpp"
@ONLY
)
Copy link
Contributor

@dg0yt dg0yt Apr 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

vpckg configures debug and release variant in parallel. This is incompatible with generating any files in the source dir at configuration time. (Race!) Ideally, generate files only in the binary dir, or not at all.
As a mitigation, you can supply DISABLE_PARALLEL_CONFIGURE to vcpkg_cmake_configure.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dg0yt for the explanation!
I will fix that in a future release if you are ok with that.
Copy link
Contributor

Choose a reason for hiding this comment

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

Then don't patch for quotes now, but add DISABLE_PARALLEL_CONFIGURE.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I don't make the debug version anymore, I don't think I have to set DISABLE_PARALLEL_CONFIGURE, right ?
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory it might be obsolete. In practice vcpkg still sets a CMake option which forbids source writes.
dg0yt
dg0yt suggested changes Apr 23, 2023
View reviewed changes
ports/samurai/portfile.cmake Show resolved Hide resolved
ports/samurai/portfile.cmake Outdated Show resolved Hide resolved
ports/samurai/portfile.cmake Outdated Show resolved Hide resolved
ports/samurai/portfile.cmake Outdated Show resolved Hide resolved
ports/samurai/portfile.cmake Outdated Show resolved Hide resolved
FrankXie05
FrankXie05 previously approved these changes Apr 23, 2023
View reviewed changes
@FrankXie05 FrankXie05 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Apr 23, 2023
dg0yt
dg0yt suggested changes Apr 23, 2023
View reviewed changes
ports/samurai/portfile.cmake Outdated Show resolved Hide resolved
@FrankXie05 FrankXie05 removed the info:reviewed Pull Request changes follow basic guidelines label Apr 23, 2023
dg0yt
dg0yt reviewed Apr 23, 2023
View reviewed changes
ports/samurai/portfile.cmake Outdated
Comment on lines 15 to 22
vcpkg_cmake_get_vars(cmake_vars_file)
include(${cmake_vars_file})

vcpkg_cmake_configure(
SOURCE_PATH "${SOURCE_PATH}"
OPTIONS
-DFETCHCONTENT_FULLY_DISCONNECTED=OFF
-DCMAKE_SYSTEM_PROCESSOR=${VCPKG_DETECTED_CMAKE_SYSTEM_PROCESSOR}
Copy link
Contributor

Choose a reason for hiding this comment

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

On a second thought, the whole CMAKE_SYSTEM_PROCESSOR must be removed. vcpkg_cmake_get_vars won't give any result which differs from what would be achieved by not explicitly passing CMAKE_SYSTEM_PROCESSOR.

Let's take a look at https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#portfiles-are-run-in-script-mode

The portfiles run in script mode. All target information in script mode comes from the triplet file. The target system processor is represented by VCPKG_TARGET_ARCHITECTURE, but it is not a value which is meant to be literally for CMAKE_SYSTEM_PROCESSOR. You can check the files in scripts/toolchains for some mappings.

Now you don't use the default vcpkg (sub) toolchains but want to chainload another one which takes CMAKE_SYSTEM_PROCESSOR as an input variable. In that case you should use custom (overlay) triplets which provide the necessary parameter to your toolchain via VCPKG_CMAKE_CONFIGURE_OPTIONS, or you must wrap your toolchain in a another scripts which takes care of setting this variable.

In any case, it doesn't belong into this port.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dg0yt. I will take some time to think about how to solve this issue.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a note about WindowsToolchain and VCPkg. I don't know if it is useful.

Anyway, it seems to be hard to disable the call of WindowsToolchain.
Copy link
Contributor

Choose a reason for hiding this comment

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

The note seems incorrect in key aspects. Maybe you should open a separate question here in discussion, "How to use WindowsToolchain with vcpkg".
@gouarin gouarin mentioned this pull request Apr 24, 2023
Open
@FrankXie05
Copy link
Contributor

Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review".
That way, I can be aware that you've responded since you can't modify the tags.
@FrankXie05 FrankXie05 marked this pull request as draft May 8, 2023 07:02
@gouarin
Copy link
Contributor Author

gouarin commented May 23, 2023

@dg0yt could you explain to me how to make working the Windows build using VCPKG_CHAINLOAD_TOOLCHAIN_FILE?
@dg0yt
Copy link
Contributor

dg0yt commented May 23, 2023

@dg0yt could you explain to me how to make working the Windows build using VCPKG_CHAINLOAD_TOOLCHAIN_FILE?

Keep it simpel. Just don't touch it for the official triplets. They do have working toolchain files.
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!
3 participants
@gouarin @FrankXie05 @dg0yt