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

Paraview on windows: Use htf5::hdf5 #44161

Merged
merged 2 commits into from May 21, 2024

Conversation

danlipsa
Copy link
Contributor

@danlipsa danlipsa commented May 13, 2024

Use htf5::hdf5 on Windows

Copy link

spackbot-app bot commented May 13, 2024

Hi @danlipsa! I noticed that the following package(s) don't yet have maintainers:

  • msmpi

Are you interested in adopting any of these package(s)? If so, simply add the following to the package class:

    maintainers("danlipsa")

If not, could you contact the developers of this package and see if they are interested? You can quickly see who has worked on a package with spack blame:

$ spack blame msmpi

Thank you for your help! Please don't add maintainers without their consent.

You don't have to be a Spack expert or package developer in order to be a "maintainer," it just gives us a list of users willing to review PRs or debug issues relating to this package. A package can have multiple maintainers; just add a list of GitHub handles of anyone who wants to volunteer.

@danlipsa
Copy link
Contributor Author

@spackbot fix style

Copy link

spackbot-app bot commented May 13, 2024

Let me see if I can fix that for you!

Copy link

spackbot-app bot commented May 13, 2024

I was able to run spack style --fix for you!

spack style --fix
==> Running style checks on spack
  selected: isort, black, flake8, mypy
==> Modified files
  var/spack/repos/builtin/packages/msmpi/package.py
  var/spack/repos/builtin/packages/paraview/package.py
==> Running isort checks
  isort checks were clean
==> Running black checks
All done! ✨ 🍰 ✨
2 files left unchanged.
  black checks were clean
==> Running flake8 checks
var/spack/repos/builtin/packages/msmpi/package.py:47: [F841] local variable 'dependent_module' is assigned to but never used
  flake8 found errors
==> Running mypy checks
lib/spack/spack/version/version_types.py:145: error: Argument 2 to "StandardVersion" has incompatible type "*Tuple[Tuple[Any, ...], Tuple[Any, ...]]"; expected "Tuple[Tuple[Any, ...], Tuple[Any, ...]]"  [arg-type]
lib/spack/spack/version/version_types.py:452: error: Argument 2 to "StandardVersion" has incompatible type "*Tuple[Tuple[Any, ...], Tuple[Any, ...]]"; expected "Tuple[Tuple[Any, ...], Tuple[Any, ...]]"  [arg-type]
lib/spack/spack/version/version_types.py:481: error: Argument 2 to "StandardVersion" has incompatible type "*Tuple[Tuple[Any, ...], Tuple[Any, ...]]"; expected "Tuple[Tuple[Any, ...], Tuple[Any, ...]]"  [arg-type]
Found 3 errors in 1 file (checked 625 source files)
  mypy found errors
Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I wasn't able to make any further changes, but please see the message above for remaining issues you can fix locally!

@danlipsa
Copy link
Contributor Author

@johnwparent Please review

Copy link
Contributor

@johnwparent johnwparent left a comment

Choose a reason for hiding this comment

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

This looks good, but can we either point to the patch in the VTK package here (and vis-versa), or create a centralized issue on GH so we can track the fact this is a duplicated patch for another package and any updates/follow on changes need to be made to both.

@danlipsa
Copy link
Contributor Author

danlipsa commented May 13, 2024

This looks good, but can we either point to the patch in the VTK package here (and vis-versa),

The patches are slightly different: the ParaView has an additional VTK in the path. So we cannot reuse the patch.

or create a centralized issue on GH so we can track the fact this is a duplicated patch for another package and any updates/follow on changes need to be made to both.

Or simply adding a comment on both stating that they are the same?

@johnwparent
Copy link
Contributor

johnwparent commented May 13, 2024

This looks good, but can we either point to the patch in the VTK package here (and vis-versa),

The patches are slightly different: the ParaView has an additional VTK in the path. So we cannot reuse the patch.

Right, sorry, should have been clearer, I don't mean re-use the patch, just indicate they are functionally the same and so any meaningful change made in one (or in the patch directive in the package recipe) should be made in the other.

or create a centralized issue on GH so we can track the fact this is a duplicated patch for another package and any updates/follow on changes need to be made to both.

Or simply adding a comment on both stating that they are the same?

Yes exactly.

Copy link
Contributor

@johnwparent johnwparent left a comment

Choose a reason for hiding this comment

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

LGTM

@johnwparent
Copy link
Contributor

Looks good, thanks Dan! I'll merge once the GL pipelines pass.

@danlipsa
Copy link
Contributor Author

@spackbot re-run pipeline

Copy link

spackbot-app bot commented May 17, 2024

I'm sorry, gitlab does not have your latest revision yet, I can't run that pipeline for you right now.

One likely possibility is that your PR pipeline has been temporarily deferred, in which case, it is awaiting a develop pipeline, and will be run when that finishes.

Please check the gitlab commit status message to see if more information is available.

Details
pr head: 9ef365b, gitlab commit parents: ['7f6210e', '4892c98']

@danlipsa danlipsa changed the title Paraview on windows Paraview on windows: Use htf5::hdf5 May 20, 2024
@danlipsa
Copy link
Contributor Author

@johnwparent Is this ready to be merged? Thanks!

@johnwparent johnwparent merged commit 4bf5cc9 into spack:develop May 21, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants