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

[cgns] Update to 4.3.0 #24531

Merged
merged 10 commits into from
May 11, 2022
Merged

[cgns] Update to 4.3.0 #24531

merged 10 commits into from
May 11, 2022

Conversation

panda-z
Copy link
Contributor

@panda-z panda-z commented May 4, 2022

Describe the pull request

  • What does your PR fix?

    Update cgns to 4.3.0

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    Yes (As before)

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@ghost
Copy link

ghost commented May 4, 2022

CLA assistant check
All CLA requirements met.

ports/cgns/hdf5.patch Outdated Show resolved Hide resolved
BillyONeal added 2 commits May 4, 2022 17:01
* Use rename rather than copy and remove
* Add quotes
* Guard explicit references to debug in case the user is using a release-only triplet.
* Use in lists
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.

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/cgns/vcpkg.json

Valid values for the license field can be found in the documentation

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.

I was unable to build cgns[hdf5,lfs,legacy,cgnstools]:x64-windows, with the failure:

-- Could NOT find Tclsh (missing: TCL_TCLSH) 
-- Could NOT find TCL (missing: TCL_LIBRARY TCL_INCLUDE_PATH) 
-- Could NOT find TCLTK (missing: TCL_LIBRARY TCL_INCLUDE_PATH TK_LIBRARY TK_INCLUDE_PATH) 
-- Could NOT find TK (missing: TK_LIBRARY TK_INCLUDE_PATH) 
CMake Error at src/cgnstools/CMakeLists.txt:80 (message):
  The path and library needs to be defined for:

      TCL;TK

It looks like some dependencies are missing from declarations?

I pushed some other minor nitpicks.

ports/cgns/vcpkg.json Show resolved Hide resolved
ports/cgns/vcpkg.json Outdated Show resolved Hide resolved
ports/cgns/vcpkg.json Outdated Show resolved Hide resolved
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label May 5, 2022
ports/cgns/portfile.cmake Outdated Show resolved Hide resolved
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: Local changes detected for cgns but no changes to version or port version.
-- Version: 4.3.0
-- Old SHA: a7a3ae05ee5762213844ecee9f048e5534d99ffc
-- New SHA: 880e06dd7731cb1848c6e627992213e7e95e0749
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/cgns/vcpkg.json

Valid values for the license field can be found in the documentation

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.

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/cgns/vcpkg.json

Valid values for the license field can be found in the documentation

@JonLiu1993
Copy link
Member

@panda-z ,I tested the features locally with the command "./vcpkg install cgns[hdf5,lfs,legacy,cgnstools]:x64-windows" failed:

CMake Error at src/cgnstools/CMakeLists.txt:80 (message):
  The path and library needs to be defined for:

      TCL;TK

@panda-z
Copy link
Contributor Author

panda-z commented May 6, 2022

@JonLiu1993

Currently, the feature "cgnstools" relies on tcl and tk as dependencies. While tcl is available in vcpkg repo but cannot be located by cmake when building cgns. Tk is not currently available in vcpkg repo (#7648).

By installing ActiveTcl one can workaround the build failure issue. But that will pull in a third party binary package. I have no idea whether we should keep this feature option or not.

@JonLiu1993
Copy link
Member

@JonLiu1993

Currently, the feature "cgnstools" relies on tcl and tk as dependencies. While tcl is available in vcpkg repo but cannot be located by cmake when building cgns. Tk is not currently available in vcpkg repo (#7648).

By installing ActiveTcl one can workaround the build failure issue. But that will pull in a third party binary package. I have no idea whether we should keep this feature option or not.

Thanks for the clarification, as you said we cannot use the feature "cgnstools" directly at the moment, I suggest to delete this feature for now

@Neumann-A
Copy link
Contributor

You cannot delete existing features... this breaks the upgrade path.

@panda-z
Copy link
Contributor Author

panda-z commented May 6, 2022

@Neumann-A

The "cgnstools" is not an existing feature. It is a new feature which is introduced by this PR.

However, "tools" is an existing feature which will be removed, since it is an "dummy" feature which is unused in portfile.cmake.

If this breaks the upgrade path, I can revert the changes.

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.

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/cgns/vcpkg.json

Valid values for the license field can be found in the documentation

@BillyONeal
Copy link
Member

You cannot delete existing features... this breaks the upgrade path.

I think it's borderline because it's a non-default feature with no effects so presumably people wouldn't have installed it. On the other hand there seems to be little strong reason to remove it either.

@panda-z I think we should describe any features with no effects with "No effects, preserved for upgrade compatibility." or similar and not add new features that are broken.

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.

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/cgns/vcpkg.json

Valid values for the license field can be found in the documentation

@panda-z
Copy link
Contributor Author

panda-z commented May 7, 2022

@BillyONeal Thanks for clarification. That makes sense. I've modified the description to "No effects, preserved for upgrade compatibility".

In addition, "cgnstools" is removed due to the lack of support for tcl/tk in vcpkg.

@JonLiu1993
Copy link
Member

@panda-z ,I tested the features locally with the command "./vcpkg install cgns[*]" failed:

CMake Error: Error required internal CMake variable not set, cmake may not be built correctly.
Missing variable is:
CMAKE_Fortran_PREPROCESS_SOURCE
CMake Error: Error required internal CMake variable not set, cmake may not be built correctly.
Missing variable is:
CMAKE_Fortran_PREPROCESS_SOURCE
CMake Error at F:/Feature-test/cgns/vcpkg/downloads/tools/cmake-3.22.2-windows/cmake-3.22.2-windows-i386/share/cmake-3.22/Modules/FortranCInterface/Detect.cmake:41 (try_compile):
  Failed to generate test project build system.
Call Stack (most recent call first):
  F:/Feature-test/cgns/vcpkg/downloads/tools/cmake-3.22.2-windows/cmake-3.22.2-windows-i386/share/cmake-3.22/Modules/FortranCInterface.cmake:224 (include)
  CMakeLists.txt:168 (include)

@panda-z
Copy link
Contributor Author

panda-z commented May 9, 2022

@JonLiu1993 This error may relate to the "not yet implemented" fortran feature.

@JonLiu1993
Copy link
Member

@JonLiu1993 This error may relate to the "not yet implemented" fortran feature.

OK, I will test the other features again

@panda-z
Copy link
Contributor Author

panda-z commented May 11, 2022

Can we remove the requires:author-response label? Thanks.

@JonLiu1993
Copy link
Member

Except for the fortran feature, all features are tested successfully in the following triplet:

  • x86-windows
  • x64-windows
  • x64-windows-static

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label May 11, 2022
@BillyONeal BillyONeal merged commit a563603 into microsoft:master May 11, 2022
@BillyONeal
Copy link
Member

Thanks for the update! (If you'd be willing to submit a PR removing that patch if unnecessary that would be nice, but didn't seem worth blocking the version update for that)

@panda-z
Copy link
Contributor Author

panda-z commented May 12, 2022

Thanks for merging!

@panda-z panda-z deleted the update-cgns branch May 12, 2022 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants