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

Hdf5 For Windows #29826

Closed
wants to merge 67 commits into from
Closed

Hdf5 For Windows #29826

wants to merge 67 commits into from

Conversation

jpopelar
Copy link
Contributor

Includes packaging for hdf5, bzip2, boost, pcre, openblas and sqlite. Also provides the WindowsPackage build system, which allows developers to specify shared, static, and static multithreaded runtime variants of packages.

This branch was rebased on top of develop moments ago so everything should be in order once tests pass

@spackbot-app

This comment was marked as off-topic.

@scheibelp scheibelp self-assigned this Mar 31, 2022
Copy link
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.

This is a preliminary review with some questions.

I think this could potentially be split into multiple PRs (e.g. the hdf5 changes are useful without the openblas changes), and that may be preferable if resolving all the questions here takes too much time.

var/spack/repos/builtin/packages/boost/package.py Outdated Show resolved Hide resolved
lib/spack/spack/build_systems/python.py Outdated Show resolved Hide resolved
lib/spack/llnl/util/filesystem.py Outdated Show resolved Hide resolved
lib/spack/spack/compilers/msvc.py Outdated Show resolved Hide resolved
lib/spack/spack/spec.py Outdated Show resolved Hide resolved
var/spack/repos/builtin/packages/boost/package.py Outdated Show resolved Hide resolved
var/spack/repos/builtin/packages/bzip2/package.py Outdated Show resolved Hide resolved

depends_on('mpi', when='+mpi')
if sys.platform != 'win32':
depends_on('mpi', when='+mpi')
Copy link
Member

Choose a reason for hiding this comment

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

(question) to be clear, does this mean that building hdf5 with MPI support on Windows does not require any install of MPI on the system?

Copy link
Member

Choose a reason for hiding this comment

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

Ping - I would have assumed that building hdf5+mpi on Windows would require an MPI installation.

@@ -414,7 +421,8 @@ def ensure_parallel_compiler_wrappers(self):
if self.spec.satisfies('@1.8.21:1.8.22,1.10.2:1.10.7,1.12.0+mpi'):
with working_dir(self.prefix.bin):
# No try/except here, fix the condition above instead:
symlink('h5cc', 'h5pcc')
if sys.platform != 'win32':
symlink('h5cc', 'h5pcc')
Copy link
Member

Choose a reason for hiding this comment

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

(question) #27021 added some logic for symlink analogs on Windows: does this still need to be skipped on Windows?

If so, the following if block also calls symlink and should also probably change in that case.

Copy link
Member

Choose a reason for hiding this comment

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

ping

var/spack/repos/builtin/packages/openblas/package.py Outdated Show resolved Hide resolved
jpopelar pushed a commit to Tech-XCorp/spack that referenced this pull request Apr 12, 2022
@spackbot-app spackbot-app bot added the documentation Improvements or additions to documentation label Apr 14, 2022
@alalazo
Copy link
Member

alalazo commented May 6, 2022

Would it be worth / possible to delay this until #30411 is in? That PR would allow to distinguish based on platform=windows in specs, rather than in-Python:

if is_windows:
    ...
else:
    ...

@scheibelp

This comment was marked as off-topic.

@scheibelp
Copy link
Member

Close/reopen to restart tests

@scheibelp scheibelp closed this May 26, 2022
@scheibelp scheibelp reopened this May 26, 2022
@spackbot-app spackbot-app bot added the stage label May 26, 2022
@jpopelar
Copy link
Contributor Author

jpopelar commented Jun 2, 2022

Test restart

@jpopelar jpopelar closed this Jun 2, 2022
@jpopelar jpopelar reopened this Jun 2, 2022
jpopelar pushed a commit to Tech-XCorp/spack that referenced this pull request Jun 15, 2022
@jpopelar jpopelar mentioned this pull request Jun 15, 2022
@jpopelar
Copy link
Contributor Author

Migrated to #31141 on fresh branch

@jpopelar jpopelar closed this Jun 15, 2022
jpopelar pushed a commit to Tech-XCorp/spack that referenced this pull request Aug 9, 2022
jpopelar pushed a commit to Tech-XCorp/spack that referenced this pull request Aug 29, 2022
jpopelar pushed a commit to Tech-XCorp/spack that referenced this pull request Sep 7, 2022
jpopelar pushed a commit to Tech-XCorp/spack that referenced this pull request Sep 13, 2022
jpopelar pushed a commit to Tech-XCorp/spack that referenced this pull request Sep 23, 2022
jpopelar pushed a commit to Tech-XCorp/spack that referenced this pull request Sep 28, 2022
jpopelar pushed a commit to Tech-XCorp/spack that referenced this pull request Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Spack on Windows
In Progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants