Support for Windows development environment by johnwparent · Pull Request #33021 · spack/spack

johnwparent · 2022-10-05T15:18:26Z

This establishes a base of support for a Windows development environment and introduces some fundamental Windows tools to Spack as packages and externally locatable components. This will support packages relying on low level Windows development components such as the Windows OpenGL driver, third party GL drivers, the driver development kit, and the Windows software development kit. This components will introduce new support or expand support for such packages as MSMPI, HDF5, Glew, Paraview, and VTK.

spackbot-app · 2022-10-05T15:58:29Z

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

wdk
wgl
win-sdk
win-wdk

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

    maintainers = ['johnwparent']

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 wdk

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.

scheibelp

I mostly have comments about readability, although there appears to be a couple issues (more detail in comments).

lib/spack/llnl/util/symlink.py

lib/spack/spack/detection/common.py

var/spack/repos/builtin/packages/win-sdk/package.py

var/spack/repos/builtin/packages/win-wdk/package.py

lib/spack/spack/operating_systems/windows_os.py

lib/spack/spack/fetch_strategy.py

var/spack/repos/builtin/packages/wgl/package.py

scheibelp · 2022-11-03T17:32:42Z

var/spack/repos/builtin/packages/win-wdk/package.py

+    def install(self, spec, prefix):
+        install_args = ["/features", "+", "/quiet", "/installpath", self.prefix]
+        with working_dir(self.stage.source_path):
+            Executable("wdksetup.exe")(*install_args)


You mentioned that users can't install wdk if it's already on the system: could the error message be augmented with a suggestion to run spack external find --not-buildable wdk?

I tested this, and the installer will report that scenario to the users prior to closure without Spack needing to do so. When spack runs Executable("wdksetup.exe")(*install_args) execution is handed over to the Windows installer and gui, which then detects other installations of the WDK and reports to that effect. Context is not returned to spack until a user acknowledges that scenario, so I don't believe reporting that would be necessary. Additionally, I don't know if the installer returns enough info to Spack via stdout that we would be able to differentiate that failure from another.

Now that this PR includes windows_registry could this be updated? That doesn't have to happen in this PR, I just wanted to check if that made this possible (I assume that for this to make sense, it would also need to run -unattended, if that or an equivalent option is available for the install).

Yes, we would be able to search for the registry key and report the already installed version. Windows installer APIs are not consistent across packages, each package creates their own. The WDK does not have the -unattended flag, instead the -quiet flag that I've already added handled both the normal interface and any error messages that may crop up. There are two to dos here, the first is the registry stuff, and the second is piping errors to the spack log file. I'll try to do both in this PR.

If you point the log at spack's log it blows things away, that will be saved for a later PR.

var/spack/repos/builtin/packages/win-wdk/package.py

lib/spack/spack/detection/common.py

lib/spack/spack/util/windows_registry.py

scheibelp

Some preliminary comments. Some are more straightforward changes and others might merit discussion. Let me know if you agree.

var/spack/repos/builtin/packages/wgl/package.py

scheibelp · 2022-11-15T19:00:11Z

var/spack/repos/builtin/packages/win-wdk/package.py

+    def install(self, spec, prefix):
+        install_args = ["/features", "+", "/quiet", "/installpath", self.prefix]
+        with working_dir(self.stage.source_path):
+            Executable("wdksetup.exe")(*install_args)


Now that this PR includes windows_registry could this be updated? That doesn't have to happen in this PR, I just wanted to check if that made this possible (I assume that for this to make sense, it would also need to run -unattended, if that or an equivalent option is available for the install).

lib/spack/spack/util/windows_registry.py

lib/spack/spack/detection/common.py

lib/spack/spack/util/windows_registry.py

scheibelp

Additional requests - hopefully all minor. Let me know if you disagree.

lib/spack/spack/compilers/msvc.py

lib/spack/spack/detection/common.py

lib/spack/spack/util/windows_registry.py

var/spack/repos/builtin/packages/perl/package.py

Update spack external find heuristics to account for Windows drivers Update external find for Windows kits

scheibelp

Some additional requests: if any of them don't seem minor let me know.

lib/spack/spack/compilers/msvc.py

lib/spack/spack/detection/path.py

lib/spack/spack/util/windows_registry.py

var/spack/repos/builtin/packages/wgl/package.py

scheibelp · 2022-11-22T00:41:39Z

Seems like the build tests are still hitting some intermittent download issues. I'll retry the pipeline.

scheibelp · 2022-11-22T00:41:52Z

@spackbot run pipeline

spackbot-app · 2022-11-22T00:41:56Z

I've started that pipeline for you!

haampie · 2022-12-05T12:03:07Z

lib/spack/llnl/util/symlink.py

    """
    if not is_windows or _win32_can_symlink():
-        os.symlink(real_path, link_path)
+        os.symlink(real_path, link_path, target_is_directory=os.path.isdir(real_path))


Why was this added? ~~It's a really bad idea, cause this way dangling symlinks cannot be created.~~ Ok, I mistakenly thought isdir throws if the file doesn't exist. In any case, I submitted #34321.

In general I'd like to ask you to never introduce changes for Windows support that cause performance regressions for non-Windows. Just assume that every file system operation is the bottleneck in some part of Spack, whether it's views, post-install hooks, buildcaches, ...

In general I'd like to ask you to never introduce changes for Windows support that cause performance regressions for non-Windows.

This request is impossible to satisfy, or even to break down into something realistic or manageable. It is equivalent to saying "don't introduce bugs" (I'm already trying to do that).

As for the specific issue, I personally think a mistake at this level is unavoidable. Even if your original complaint was true. We have this code, and then we have you noticing and making an adjustment that avoids the issue (presumably because you have a benchmark which detects that something started slowing down). If you're worried about dangling symlinks and the tests don't have it, then they should have it; if you think there should be a benchmark for tracking this type of slowdown and we don't have it, then we should have one.

I guess what I'm saying is: be mindful about filesystem operations.

* Add a WindowsRegistryView class, which can query for existing package installations on Windows. This is particularly important because some Windows packages (including those added here) do not allow two simultaneous installs, and this can be queried in order to provide a clear error message. * Consolidate external path detection logic for Windows into WindowsKitExternalPaths and WindowsCompilerExternalPaths objects. * Add external-only packages win-sdk and wgl * Add win-wdk (including external detection) which depends on win-sdk * Replace prior msmpi implementation with a source-based install (depends on win-wdk). This install can control the install destination (unlike the binary installation). * Update MSVC compiler to choose vcvars based on win-sdk dependency * Provide "msbuild" module-level variable to packages during build * When creating symlinks on Windows, need to explicitly specify when a symlink target is a directory * executables_in_path no-longer defaults to using PATH (this is now expected to be taken care of by the caller)

alalazo · 2023-09-05T08:43:57Z

lib/spack/spack/detection/common.py

+            return []
+        program_files = os.environ["PROGRAMFILES(x86)"]
+        kit_base = os.path.join(
+            program_files, "Windows Kits", WindowsKitExternalPaths.plat_major_ver
+        )
+        return kit_base


Is there any reason this function returns either an empty list or a string?

Nope, and that's definitely a mistake we missed. IMO we should actually always be returning a list from a glob over the base kits directory so we can support SDK 10 and SDK 11.
I'll refactor, PR, and tag you.

No worries, I stumbled on this while doing #39803

johnwparent marked this pull request as draft October 5, 2022 15:18

spackbot-app bot added compilers conflicts core PR affects Spack core functionality dependencies new-package new-version utilities labels Oct 5, 2022

johnwparent mentioned this pull request Oct 27, 2022

Overhaul certain spack packages #33564

Closed

scheibelp self-assigned this Nov 3, 2022

johnwparent force-pushed the win/external-find-drivers branch from 8988753 to 837d81f Compare November 3, 2022 16:18

spackbot-app bot added architecture fetching headers libraries virtual-dependencies labels Nov 3, 2022

scheibelp requested changes Nov 3, 2022

View reviewed changes

johnwparent marked this pull request as ready for review November 3, 2022 19:01

spackbot-app bot added update-package build-environment labels Nov 15, 2022

johnwparent requested a review from scheibelp November 15, 2022 01:34

scheibelp reviewed Nov 15, 2022

View reviewed changes

lib/spack/spack/util/windows_registry.py Outdated Show resolved Hide resolved

johnwparent force-pushed the win/external-find-drivers branch from bedc37a to 0177f74 Compare November 15, 2022 18:54

spackbot-app bot added new-variant patch labels Nov 15, 2022

scheibelp requested changes Nov 15, 2022

View reviewed changes

scheibelp requested changes Nov 17, 2022

View reviewed changes

johnwparent added 2 commits November 17, 2022 14:05

Update external find heuristics

3f85475

Update spack external find heuristics to account for Windows drivers Update external find for Windows kits

add driver packages

6b5f166

johnwparent added 4 commits November 18, 2022 11:11

Rm merge cruft

d3f2ade

No from_executables default

112b55c

Registry interface updates

d446c2a

MSMPI improved fortran handling

e90d0f1

spackbot-app bot added commands tests General test capability(ies) labels Nov 18, 2022

johnwparent added 3 commits November 18, 2022 11:24

PSFBLACK

239d9e1

Review changes

fcfca1d

Proper location for setup_build_environment

9d2afac

scheibelp requested changes Nov 19, 2022

View reviewed changes

johnwparent added 4 commits November 18, 2022 20:48

Partial review updates

41edd49

WGL to non buildable

dac72a0

Fix broken test

0bc1ad8

style

3819c4f

scheibelp previously approved these changes Nov 19, 2022

View reviewed changes

Fixup broken test in external find

945ca90

johnwparent dismissed scheibelp’s stale review via 945ca90 November 20, 2022 22:46

This comment was marked as outdated.

Sign in to view

scheibelp approved these changes Nov 22, 2022

View reviewed changes

scheibelp merged commit 793a7bc into spack:develop Nov 22, 2022

greenc-FNAL mentioned this pull request Dec 1, 2022

Sync with upstream FNALssi/spack#72

Closed

haampie reviewed Dec 5, 2022

View reviewed changes

haampie mentioned this pull request Dec 5, 2022

Avoid stat call in llnl.util.symlink on non-windows #34321

Merged

alalazo reviewed Sep 5, 2023

View reviewed changes

alalazo mentioned this pull request Sep 11, 2023

Expand Windows EXE detection regex #39850

Merged

Conversation

johnwparent commented Oct 5, 2022

Uh oh!

spackbot-app bot commented Oct 5, 2022

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

scheibelp commented Nov 22, 2022

Uh oh!

scheibelp commented Nov 22, 2022

Uh oh!

spackbot-app bot commented Nov 22, 2022

Uh oh!

haampie Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haampie Dec 5, 2022 •

edited

Loading