Skip to content

Hdf5 For Windows V2#31141

Merged
scheibelp merged 133 commits intospack:developfrom
Tech-XCorp:windows-hdf5-v2
Nov 17, 2022
Merged

Hdf5 For Windows V2#31141
scheibelp merged 133 commits intospack:developfrom
Tech-XCorp:windows-hdf5-v2

Conversation

@jpopelar
Copy link
Copy Markdown
Contributor

Remake of PR #29826 due to branch conflicts and duplicated commits

@jpopelar jpopelar mentioned this pull request Jun 15, 2022
Copy link
Copy Markdown
Member

@scheibelp scheibelp left a comment

Choose a reason for hiding this comment

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

Let me know if any of these requests seem to miss the point.

@scheibelp
Copy link
Copy Markdown
Member

@jpopelar can you push an empty commit here (see also: #31589 (comment)).

@jpopelar
Copy link
Copy Markdown
Contributor Author

Just pushed up a commit to trim out windows/msvc_variants and addressed your feedback @scheibelp. I'll let the tests go again and LMK if you notice anything else out of the ordinary!

@scheibelp
Copy link
Copy Markdown
Member

#31265 looks related to the style failure, if applying the fix described there works, then could you mention that in that issue?

(one thing that's strange: that issue describes how this would come up for non-Ubuntu systems, but I checked the runner for this style check and it appears to be Ubuntu, so I'm not sure how this would come up there - it is the same error message though)

@jpopelar
Copy link
Copy Markdown
Contributor Author

77796a6 does not look like it did it; just applied @alalazo's suggestion to bootstrap.py by adding a .local into the bin_dir path. Are there any other issues that discuss this topic?

@jpopelar
Copy link
Copy Markdown
Contributor Author

jpopelar commented Aug 5, 2022

@scheibelp Pinging you as per your requests. I'll look into the conflicts if you want to examine anything else

Copy link
Copy Markdown
Member

@scheibelp scheibelp left a comment

Choose a reason for hiding this comment

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

I have a few questions/requests. I'll do a more-thorough review once this is updated with the latest develop.

@jpopelar
Copy link
Copy Markdown
Contributor Author

@spackbot run pipeline

@spackbot-app
Copy link
Copy Markdown

spackbot-app bot commented Nov 16, 2022

I've started that pipeline for you!

@scheibelp scheibelp merged commit 381bedf into spack:develop Nov 17, 2022
@scheibelp
Copy link
Copy Markdown
Member

Thanks @jpopelar!

@jpopelar
Copy link
Copy Markdown
Contributor Author

You're welcome @scheibelp, have a good Thanksgiving

Comment on lines +206 to +208
# Skip this on Windows since pkgconfig is autotools
for plat in ["cray", "darwin", "linux"]:
depends_on("pkgconfig", when="platform=%s" % plat, type="run")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm late with my comments but nonetheless.

  1. I disagree with the comment: pkgconfig is not autotools.
  2. The compiler wrappers will not work without pkg-config in the PATH.

I understand that the compiler wrappers of hdf5 a rarely used but if pkg-config works on Windows, the code above should be reverted. Otherwise, the wrappers should be removed after the installation to avoid confusion.

amd-toolchain-support pushed a commit to amd-toolchain-support/spack that referenced this pull request Feb 16, 2023
* Enable hdf5 build (including +mpi) on Windows
* This includes updates to hdf5 dependencies openssl (minor edit) and
  bzip2 (more-extensive edits)
* Add binary-based installation of msmpi (this is currently the only
  supported MPI implementation in Spack for Windows). Note that this
  does not install to the Spack-specified prefix. This implementation
  will be replaced with a source-based implementation

Co-authored-by: John Parent <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants