Conversation
|
@BetsyMcPhail @zackgalbreath @tgamblin @jpopelar PTAL when convenient. This PR marks the first batch of Windows specific changes ready for integration into Spack's develop branch. |
There was a problem hiding this comment.
I went over the PR, and checked what changes are needed to some of the packages. In general it looks there will be almost two different build system styles: Windows and non-Windows. That is pretty common in Conda also. I think that's a good start. Later one can extract as much of this platform specific logic as possible into Spack itself, such automatically appending .exe on Windows (see my other comment below).
The CI now runs this on Windows and it seems to work.
Does this run in the cmd.exe console?
I don't see any fundamental issues with the approach. This seems like exactly what I would like to use.
Thanks for this work, it's very exciting to get Spack working on Windows as a first class package manager.
| install('ninja', prefix.bin) | ||
| name = 'ninja' | ||
| if sys.platform == 'win32': | ||
| name = name + '.exe' |
There was a problem hiding this comment.
Should this be folded into Spack itself? It seems this change will be very common for most packages.
There was a problem hiding this comment.
More to the point, why not use the cmake install target and just not do this manually at all? The ninja CMakeLists.txt will install that file: https://github.com/ninja-build/ninja/blob/master/CMakeLists.txt#L237
There was a problem hiding this comment.
@certik This command is copying a file, I don't know what we want to assume a file extension if that's what you mean. In other places care has been taken to take care of Windows specifics like this behind the scenes, but in this case (and in the case of many bespoke install methods) that logic will need to be handled here.
@trws as CMake depends_on ninja on Windows, Ninja cannot leverage CMake here unfortunately.
This can run on a PWSH or CMD console. It's currently run on the PWSH console on CI, and I personally run CMD locally. I believe executable extension handling for Windows has been incorporated into most of Spack, however, it looks like certain specific situations may have been overlooked (i.e. the Ninja example referenced). |
|
Awesome. It looks like it is designed well. I need to think how I can start incorporating Spack into my workflows, I have been mostly invested into Conda so far simply since I needed to distribute my software on Windows. Probably the best way I can start using Spack is to setup some GitHub project that tests my software on Windows, macOS and Linux using Spack, and start recommending Spack to users with exact instructions how to use on every platform. |
I will note that a getting started guide has been added in this PR for Windows users of Spack. Further documentation on topics such as "Transitioning a package to support Windows" could be generated if there was a significant need. For Windows users, our CI generates an MSI installer for Spack itself, which I believe is how Anaconda does things. |
adamjstewart
left a comment
adamjstewart
left a comment
There was a problem hiding this comment.
We've been thinking a lot lately about how to handle packages that change build system from one version to the next https://spack.readthedocs.io/en/latest/build_systems/multiplepackage.html but we haven't thought much about packages that require different build systems on different OSes. I think the solution may end up being very similar.
| @@ -0,0 +1,204 @@ | |||
| name: windows tests | |||
There was a problem hiding this comment.
@alalazo I wonder if we could combine a lot of these platform-specific workflows into a single workflow with a matrix over different platforms. There shouldn't be a ton of differences between platforms other than installing dependencies, right?
There was a problem hiding this comment.
I'm happy to take a pass at this in this PR if time permits, or at a later date. When implementing I initially created a prototype to that effect that I can publish if need be.
lib/spack/docs/getting_started.rst
Outdated
| update your ``PATH`` variable to include the ``git`` command. | ||
|
|
||
| """" | ||
| Perl |
There was a problem hiding this comment.
All Spack packages that depend on Perl are required to list a dependency on Perl. There shouldn't be anything in Spack that requires Perl, right?
There was a problem hiding this comment.
This may be a vestige leftover from before Spack could install Perl on Windows and I believe was required to support some of the LaPack functionality, I'll be looking into this.
| config('Configure', | ||
| '--prefix=%s' % prefix, | ||
| '--openssldir=%s' % join_path(prefix, 'etc', 'openssl'), | ||
| 'CC=\"%s\"' % os.environ.get('SPACK_CC'), |
There was a problem hiding this comment.
This shouldn't need quotes or escapes, we aren't running this in a shell like bash that treats these differently, we're running it in Python
There was a problem hiding this comment.
Why is this split in two? All ./config does is exec Configure. These should be the same, just with the appropriate option on the end to set it to windows.
Admittedly I'm a little out of the loop on the multi-build system packaging effort, but in supporting an additional OS, we often had to transition the build system, so the solutions do seem like they would converge on the same set of solutions. On a related note, I personally would have found the ability to express the concept of What are your thoughts on more Windows specific documentation, especially w.r.t. transitioning packages to support Windows. Working through your comments now. |
| '3': 'https://cmake.org/files/v3.21/cmake-3.21.2-windows-x86_64.zip', | ||
| '2': 'https://cmake.org/files/v2.8/cmake-2.8.4-win32-x86.zip' |
There was a problem hiding this comment.
I get that cmake needs cmake to bootstrap on windows, as unfortunate as that may be, but is the plan to use upstream binaries rather than something spack managed? It seems quite unlike the solutions used for other bootstrapping issues.
| bootstrap = Executable('./bootstrap') | ||
| bootstrap(*bootstrap_args) | ||
|
|
||
| def build(self, spec, prefix): |
There was a problem hiding this comment.
This and the two like it below make me think this really ought to be, ironically, a CmakePackage derived class that adds a pre-configure stage to do the bootstrap phase. That removes much of the need for the variant, and takes out all this checking. Should also help if we ever decide to support other generators.
There was a problem hiding this comment.
CMakePackage has a cmake phase, and much of its benefit revolves around organizing args etc. for that phase, so I think this might be confusing.
There was a problem hiding this comment.
This build phase is not designed to specify arguments to cmake at what in CMakePackages would be the cmake phase but to ninja in the build phase. I think if anything it would be better if anything to rename the bootstrap phase above, but honestly I don't think that accurately describes what's going on because CMake is bootstrapping itself, as was indicated by the name prior to this pr.
| if not is_windows: | ||
| return | ||
| win_install_path = os.path.join(self.prefix.bin, "MSWin32") | ||
| if self.host_is_64bit(): |
There was a problem hiding this comment.
This looks a lot like functionality that needs to be factored out for re-use.
There was a problem hiding this comment.
The last for loop may be worth refactoring, but a larger proportion of this code is highly specific to the Perl build structure, which is non standard on Windows (not that there's much of a standard to begin with)
| """ | ||
| # Windows builds use a batch script to drive | ||
| # configure and build in one step | ||
| with working_dir(self.build_directory): |
There was a problem hiding this comment.
How is this usage avoiding the auto-download of external dependencies? My impression was that the build.bat file downloads everything if it doesn't exist, and the msbuild build and install doesn't. That also begs the question, why all the custom install logic rather than using the pcbuild.proj with msbuild?
There was a problem hiding this comment.
I don't believe it was ever stated that it does avoid that, if it was I apologize, as that is misleading. The build.bat was chosen explicitly because it does download the external dependencies, a number of which are not currently supported by Spack on Windows.
|
Ok... A full review on this is going to take a while, but first impression is that I like it, but I have a couple high-level questions and a thought.
|
|
+1 for ninja as default on windows. I think we should try to make the assumption that key toolchain components like cmake are bootstrapped or preinstalled: cmake + ninja especially. |
Great point. I would say MSVC is the priority, but I assumed that it would support clang and any other compiler eventually, just like on Linux or macOS you can select any compiler. |
|
I would add Intel to the list as well -- neither clang nor MSVC have standalone Fortran compilers at the moment if I'm not mistaken. |
|
When it comes to Fortran, we are developing @lfortran, and it works with MSVC and mingw. It's not production ready yet, but we are hoping it will soon. I want to make sure it works great with Spack. |
|
That cmake stuff should really be done differently indeed... spack should never just download an arbitrary binary and run it without user consent, and in particular in this case it's not even doing checksum validation :/ |
Right now we have tested with the Intel Fortran compiler+MSVC to build LAPACK. Using the Intel compiler would be another good addition. However, Intel Fortran has been tested and is known to work. |
MSVC can be installed with a toolkit that provides CMake and Ninja executables for use (on my machine I have v3.20 and v1.9.6, respectively). If the user doesn't have it, that can be easily remedied by rerunning the installer. In other words, this is not an outlandish assumption to make |
I think the idea is to eventually use a spack binary repository to serve up the initial cmake. Ninja can be built from source in spack now. For now we are able to bootstrap cmake with the binary from www.cmake.org adding checksum has been done locally and will be added to this PR. |
|
Consistency nitpick: spack on linux/macos has all lowercase values for |
#27021 broke fetching for CVS-based packages because: - The mirror logic was using URL parsing to extract a path from the CVS repository location - #27021 added sanity checks to enforce that strings passed to the URL parser were actually URLs This replaces the call to "url_util.parse" with logic that is customized for CVS. This implies that VCSFetchStrategy should rename the "url_attr" attribute to something more generic, but that should be handled separately.
spack#27021 broke fetching for CVS-based packages because: - The mirror logic was using URL parsing to extract a path from the CVS repository location - spack#27021 added sanity checks to enforce that strings passed to the URL parser were actually URLs This replaces the call to "url_util.parse" with logic that is customized for CVS. This implies that VCSFetchStrategy should rename the "url_attr" attribute to something more generic, but that should be handled separately.
spack#27021 broke fetching for CVS-based packages because: - The mirror logic was using URL parsing to extract a path from the CVS repository location - spack#27021 added sanity checks to enforce that strings passed to the URL parser were actually URLs This replaces the call to "url_util.parse" with logic that is customized for CVS. This implies that VCSFetchStrategy should rename the "url_attr" attribute to something more generic, but that should be handled separately.
* The configure script on Windows requires that CC/CXX be enclosed in quotes if the paths to those compiler executables contain spaces (so unlike most instances of Executable, the arguments need to contain the quotes) * OpenSSL requires the nasm package on Windows * Restore parallel build from 075e942 (accidentally reverted in #27021)
Resolve path/URL parsing issues introduced by spack#27021
|
Hi, Install any package on Windows e.g. spack install cpuinfo WSL2 - is both "win32" and "linux" system So, introduction of additional "platform" is required. Regards. Valeriy |
Resolve path/URL parsing issues introduced by #27021
|
Hi, To reproduce it try $spack install libx11 [+] /home/xtovo/wrk/fairsoft/linux-centos7-skylake/gcc-11.2.1/pkgconf-1.8.0-e6upgqa4gpcosaauhjqzpuloj5lspifz /home/xtovo/spack/lib/spack/spack/build_environment.py:1076, in _setup_pkg_and_run: 1076 # show that, too. See build log for details: ==> Warning: Skipping build of libxcb-1.14-2zj5z2xolzoysm7lwiwvqpv5rgjiwwxi since xcb-proto-1.14.1-5yoxwi4ms7duphpfy4q4otelm3zjym63 failed Mess by mix of fork/spawn ? Regards. Valeriy |
FWIW that often appears to be the culprit because all errors that occur during the build will include this in the stack trace. In this case though it looks like a permissions error: @x2v0 can you open this as a new issue (and ping me on it)? Note that it is easy to lose track of problems recorded in closed issues/PRs. |
|
DONE 22.06.2022, 21:36, "Peter Scheibel" ***@***.***>: seems the problems on WSL2 are still exist and connected with start_build_process in build_enviroment.pyFWIW that often appears to be the culprit because all errors that occur during the build will include this in the stack trace. In this case though it looks like a permissions error:==> Error: PermissionError: [Errno 1] Operation not permitted: '/home/xtovo/wrk/fairsoft/linux-centos7-skylake/gcc-11.2.1/xcb-proto-1.14.1-5yoxwi4ms7duphpfy4q4otelm3zjym63/lib/python3.1/site-packages/xcbgen/pycache'
@x2v0 can you open this as a new issue (and ping me on it)? Note that it is easy to lose track of problems recorded in closed issues/PRs.—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Resolve path/URL parsing issues introduced by spack#27021
| @classmethod | ||
| def detect(cls): | ||
| plat = platform.system().lower() | ||
| return 'cygwin' in plat or 'win32' in plat or 'windows' in plat |
There was a problem hiding this comment.
This breaks Cygwin support:
- Cygwin python does not have encoding 'mbcs'
- Attempts to determine the platform version on Cygwin will return the Cygwin version (3.3) not the Windows version (10 or 11)
This will crash when Windows.detect() succeeds and the loop tries to call Windows(), which calls WindowsOs(), which will pass encoding="mbcs" to subprocess.Popen in some python versions and complain if platform.release() < 10
haampie
left a comment
haampie
left a comment
There was a problem hiding this comment.
Rather inconvenient this PR made Spack depend on distutils...
| import re | ||
| import subprocess | ||
| import sys | ||
| from distutils.version import StrictVersion |
There was a problem hiding this comment.
@johnwparent / @scheibelp why did this ever pass review? It was deprecated in Python 3.10. We have our own version utilities. Now Spack is forward incompatible with Python 3.12.
There was a problem hiding this comment.
Rather inconvenient this PR made Spack depend on distutils...
This is an utterly dramatic phrasing. This PR did not entrench a distutils dependency irreparably in the depths of Spack: it is it is easily removed as an import (in the absolute worst case, that specific logic could be vendored in, but there must be many ways to handle it - as you say, we have our own version utilities).
why did this ever pass review?
I fully contest both your wording and the sentiment behind it: that we must be so hyper-vigilant that we analyze line by line all possible future problems that would be caused. That is completely untenable as an approach to developing software.
Finally, this PR was merged approximately 1.5 years ago: stop dropping random comments in dead PRs and raise an issue (feel free to reference the PR in the issue).
Introduced in spack#27021, makes Spack forward incompatible with Python. The module was already deprecated at the time of the PR.
* msvc.py: don't import distutils Introduced in #27021, makes Spack forward incompatible with Python. The module was already deprecated at the time of the PR. * update spack package
* msvc.py: don't import distutils Introduced in spack#27021, makes Spack forward incompatible with Python. The module was already deprecated at the time of the PR. * update spack package
* msvc.py: don't import distutils Introduced in spack#27021, makes Spack forward incompatible with Python. The module was already deprecated at the time of the PR. * update spack package
* msvc.py: don't import distutils Introduced in spack#27021, makes Spack forward incompatible with Python. The module was already deprecated at the time of the PR. * update spack package
14 participants
This branch adds basic Windows support for Spack.
This branch includes: