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

[OpenTURNS] add new port #29336

Merged
merged 14 commits into from
Feb 8, 2023
Merged

Conversation

Neumann-A
Copy link
Contributor

  • 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 ... controlled by USE_ variables which are all ON.
  • 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.

github-actions[bot]
github-actions bot previously approved these changes Jan 31, 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!

All manifest files must be formatted

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

Diff
diff --git a/ports/openturns/vcpkg.json b/ports/openturns/vcpkg.json
index 328fc74..d243331 100644
--- a/ports/openturns/vcpkg.json
+++ b/ports/openturns/vcpkg.json
@@ -6,8 +6,8 @@
   "license": null,
   "dependencies": [
     "blas",
-    "boost-random",
     "boost-multiprecision",
+    "boost-random",
     "ceres",
     "cminpack",
     "dlib",
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 openturns have changed but the version was not updated
version: 1.2.0
old SHA: 3b9c53996a09bfaf5f4292c0b42e820e7b5349d0
new SHA: 9f8dfa8549b68df28e313b8232800d689fd30411
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@Neumann-A Neumann-A marked this pull request as draft January 31, 2023 22:41
@MonicaLiu0311 MonicaLiu0311 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Feb 1, 2023
@Neumann-A
Copy link
Contributor Author

Ok static stuff seems to be just missing linking gmp

github-actions[bot]
github-actions bot previously approved these changes Feb 1, 2023
@Neumann-A Neumann-A marked this pull request as ready for review February 1, 2023 22:44
github-actions[bot]
github-actions bot previously approved these changes Feb 1, 2023
@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Neumann-A
Copy link
Contributor Author

@BillyONeal any hint how i should solve the osx configure error here? Seems like the bison version on the vm is too low. Only has 2.3 and 2.4 is required. This is one of the reasons why I think we need tool ports to deal with the versions of those in the same way as the rest.

github-actions[bot]
github-actions bot previously approved these changes Feb 2, 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.

@BillyONeal any hint how i should solve the osx configure error here? Seems like the bison version on the vm is too low. Only has 2.3 and 2.4 is required.

I think a baseline skip is appropriate for that.

This is one of the reasons why I think we need tool ports to deal with the versions of those in the same way as the rest.

Tool ports come with their own problems though. For instance, they expose the tool to the installed tree which is generally undesirable for toolset parts. And they effectively never take things from the system even if those are perfectly good. (They make the VCPKG_FORCE_SYSTEM_BINARIES / we fail on arm linux or alpine etc. problem worse) I'm on the hook to have a cohesive answer for fetch/tool-ports/artifacts/find_acquire_program soon, will keep you posted.

github-actions[bot]
github-actions bot previously approved these changes Feb 3, 2023
github-actions[bot]
github-actions bot previously approved these changes 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 long as the usage makes sense to you, @Neumann-A , can you confirm?

Thanks for the new port!

@Neumann-A
Copy link
Contributor Author

can you confirm?

I mean it is the same usage as the heuristics and it looked right to me considering what targets gets expored which is why i didn't bother to add a usagge.

@BillyONeal
Copy link
Member

I've been adding them just to get rid of the heuristic part of the message and asking folks who are adding ports to confirm that's what they expect. (When a new port is added hopefully the person adding knows what they wanted to do with it?)

@dg0yt
Copy link
Contributor

dg0yt commented Feb 5, 2023

When a new port is added hopefully the person adding knows what they wanted to do with it?

This may be to optimistic. People may have their own find procedures or do not use CMake at all. And even our reviewers didn't object against obviously false results (find_package(cmake REQUIURED)). However, my changes seem to have eliminated a big share of the bad output, in related to subdirectories and find modules.

@Neumann-A
Copy link
Contributor Author

I just need the port to fix the vtk all feature. As far as I can tell usage there is via variables and not targets.

@Neumann-A
Copy link
Contributor Author

@MonicaLiu0311 please remove the tag author-response (at least I don't know what to response to here.)

@MonicaLiu0311
Copy link
Contributor

  • The generated "usage text" is accurate.
openturns provides CMake targets:

    # this is heuristically generated, and may not be correct
    find_package(OpenTURNS CONFIG REQUIRED)
    target_link_libraries(main PRIVATE OT)

The usage test passed(The header files can be found.).

@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Feb 8, 2023
@JavierMatosD JavierMatosD merged commit d289dea into microsoft:master Feb 8, 2023
@Neumann-A Neumann-A deleted the add_openturns branch February 8, 2023 21:53
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