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

Windows Install tests #34696

Merged
merged 17 commits into from
Aug 14, 2023
Merged

Windows Install tests #34696

merged 17 commits into from
Aug 14, 2023

Conversation

markus-ferrell
Copy link
Contributor

@markus-ferrell markus-ferrell commented Dec 26, 2022

This PR changes the install tests to to make them windows compatible

@markus-ferrell
Copy link
Contributor Author

@spackbot fix style

@spackbot-app
Copy link

spackbot-app bot commented Jan 2, 2023

Let me see if I can fix that for you!

@spackbot-app
Copy link

spackbot-app bot commented Jan 2, 2023

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

spack style --fix
==> Running style checks on spack
  selected: isort, mypy, black, flake8
==> Modified files
  lib/spack/llnl/util/filesystem.py
  lib/spack/spack/build_environment.py
  lib/spack/spack/cmd/unit_test.py
  lib/spack/spack/installer.py
  lib/spack/spack/test/cmd/find.py
  lib/spack/spack/test/cmd/install.py
  lib/spack/spack/util/url.py
  lib/spack/spack/version.py
  var/spack/repos/builtin.mock/packages/build-error/package.py
  var/spack/repos/builtin.mock/packages/build-warnings/package.py
  var/spack/repos/builtin.mock/packages/callpath/package.py
  var/spack/repos/builtin.mock/packages/dyninst/package.py
  var/spack/repos/builtin.mock/packages/printing-package/package.py
==> Running isort checks
Fixing /tmp/tmpyinx4m1c/spack/var/spack/repos/builtin.mock/packages/build-error/package.py
Fixing /tmp/tmpyinx4m1c/spack/var/spack/repos/builtin.mock/packages/build-warnings/package.py
Fixing /tmp/tmpyinx4m1c/spack/var/spack/repos/builtin.mock/packages/callpath/package.py
Fixing /tmp/tmpyinx4m1c/spack/var/spack/repos/builtin.mock/packages/dyninst/package.py
Fixing /tmp/tmpyinx4m1c/spack/var/spack/repos/builtin.mock/packages/printing-package/package.py
Fixing /tmp/tmpyinx4m1c/spack/lib/spack/spack/version.py
  isort checks were clean
==> Running mypy checks
Success: no issues found in 571 source files
  mypy checks were clean
==> Running black checks
reformatted lib/spack/spack/build_environment.py
reformatted lib/spack/llnl/util/filesystem.py
reformatted lib/spack/spack/test/cmd/find.py
reformatted var/spack/repos/builtin.mock/packages/build-error/package.py
reformatted var/spack/repos/builtin.mock/packages/build-warnings/package.py
reformatted var/spack/repos/builtin.mock/packages/dyninst/package.py
reformatted var/spack/repos/builtin.mock/packages/printing-package/package.py
reformatted lib/spack/spack/installer.py
All done! ✨ 🍰 ✨
8 files reformatted, 5 files left unchanged.
  black checks were clean
==> Running flake8 checks
lib/spack/spack/test/cmd/find.py:9: [F401] 'sys' imported but unused
var/spack/repos/builtin.mock/packages/build-warnings/package.py:23: [E501] line too long (101 > 99 characters)
  flake8 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've updated the branch with style fixes.

@markus-ferrell
Copy link
Contributor Author

@spackbot fix style

@spackbot-app
Copy link

spackbot-app bot commented Jan 2, 2023

Let me see if I can fix that for you!

@spackbot-app
Copy link

spackbot-app bot commented Jan 2, 2023

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

spack style --fix
==> Running style checks on spack
  selected: isort, mypy, black, flake8
==> Modified files
  lib/spack/llnl/util/filesystem.py
  lib/spack/spack/build_environment.py
  lib/spack/spack/cmd/unit_test.py
  lib/spack/spack/installer.py
  lib/spack/spack/test/cmd/find.py
  lib/spack/spack/test/cmd/install.py
  lib/spack/spack/util/url.py
  lib/spack/spack/version.py
  var/spack/repos/builtin.mock/packages/build-error/package.py
  var/spack/repos/builtin.mock/packages/build-warnings/package.py
  var/spack/repos/builtin.mock/packages/callpath/package.py
  var/spack/repos/builtin.mock/packages/dyninst/package.py
  var/spack/repos/builtin.mock/packages/printing-package/package.py
==> Running isort checks
Fixing /tmp/tmp_6l2r73l/spack/var/spack/repos/builtin.mock/packages/build-error/package.py
Fixing /tmp/tmp_6l2r73l/spack/var/spack/repos/builtin.mock/packages/build-warnings/package.py
Fixing /tmp/tmp_6l2r73l/spack/var/spack/repos/builtin.mock/packages/callpath/package.py
Fixing /tmp/tmp_6l2r73l/spack/var/spack/repos/builtin.mock/packages/dyninst/package.py
Fixing /tmp/tmp_6l2r73l/spack/var/spack/repos/builtin.mock/packages/printing-package/package.py
Fixing /tmp/tmp_6l2r73l/spack/lib/spack/spack/version.py
  isort checks were clean
==> Running mypy checks
Success: no issues found in 571 source files
  mypy checks were clean
==> Running black checks
reformatted lib/spack/spack/build_environment.py
reformatted lib/spack/llnl/util/filesystem.py
reformatted lib/spack/spack/test/cmd/find.py
reformatted var/spack/repos/builtin.mock/packages/build-error/package.py
reformatted var/spack/repos/builtin.mock/packages/build-warnings/package.py
reformatted var/spack/repos/builtin.mock/packages/dyninst/package.py
reformatted var/spack/repos/builtin.mock/packages/printing-package/package.py
reformatted lib/spack/spack/installer.py
All done! ✨ 🍰 ✨
8 files reformatted, 5 files left unchanged.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> spack style checks were clean
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've updated the branch with style fixes.

@markus-ferrell
Copy link
Contributor Author

@spackbot fix style

@spackbot-app
Copy link

spackbot-app bot commented Feb 2, 2023

Let me see if I can fix that for you!

@spackbot-app
Copy link

spackbot-app bot commented Feb 2, 2023

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
  lib/spack/llnl/util/filesystem.py
  lib/spack/spack/build_environment.py
  lib/spack/spack/cmd/unit_test.py
  lib/spack/spack/installer.py
  lib/spack/spack/test/cmd/find.py
  lib/spack/spack/test/cmd/install.py
  lib/spack/spack/util/url.py
  lib/spack/spack/version.py
  var/spack/repos/builtin.mock/packages/build-error/package.py
  var/spack/repos/builtin.mock/packages/build-warnings/package.py
  var/spack/repos/builtin.mock/packages/callpath/package.py
  var/spack/repos/builtin.mock/packages/printing-package/package.py
==> Running isort checks
Fixing /tmp/tmpnjl52605/spack/var/spack/repos/builtin.mock/packages/build-error/package.py
Fixing /tmp/tmpnjl52605/spack/var/spack/repos/builtin.mock/packages/build-warnings/package.py
Fixing /tmp/tmpnjl52605/spack/var/spack/repos/builtin.mock/packages/callpath/package.py
Fixing /tmp/tmpnjl52605/spack/var/spack/repos/builtin.mock/packages/printing-package/package.py
Fixing /tmp/tmpnjl52605/spack/lib/spack/spack/version.py
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/build_environment.py
reformatted lib/spack/llnl/util/filesystem.py
reformatted lib/spack/spack/installer.py
reformatted lib/spack/spack/test/cmd/install.py
reformatted lib/spack/spack/util/url.py
reformatted var/spack/repos/builtin.mock/packages/build-error/package.py
reformatted var/spack/repos/builtin.mock/packages/build-warnings/package.py
All done! ✨ 🍰 ✨
7 files reformatted, 5 files left unchanged.
  black checks were clean
==> Running flake8 checks
lib/spack/spack/test/cmd/find.py:136: [F821] undefined name 'sys'
lib/spack/spack/test/cmd/find.py:362: [F821] undefined name 'sys'
  flake8 found errors
==> Running mypy checks
lib/spack/spack/test/cmd/find.py:362: error: Name "sys" is not defined  [name-defined]
Found 1 error in 1 file (checked 576 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've updated the branch with style fixes.

@markus-ferrell markus-ferrell marked this pull request as ready for review February 2, 2023 18:59
@@ -2034,6 +2034,9 @@ def _real_install(self):

# Catch any errors to report to logging
self.timer.start(phase_fn.name)
import time

time.sleep(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to prevent a race condition in the logger for windows. This should be removed and replaced with a logger refactor.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the error here again? I know we went through this a little while ago, but I'd like to record that conversation here. Also can we sleep for less time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error was when running test_cdash_upload_build_error. This test is skipped in this PR but this change was more here for the discussion.

The installer would output the line Executing phase install after the actual install output. This makes cdash's build output reader fail because the build output is out of order.

@markus-ferrell
Copy link
Contributor Author

@spackbot fix style

@spackbot-app
Copy link

spackbot-app bot commented Aug 10, 2023

Let me see if I can fix that for you!

@spackbot-app
Copy link

spackbot-app bot commented Aug 10, 2023

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
  lib/spack/spack/cmd/unit_test.py
  lib/spack/spack/directory_layout.py
  lib/spack/spack/test/cmd/install.py
  lib/spack/spack/test/directory_layout.py
  lib/spack/spack/test/util/util_url.py
  lib/spack/spack/test/versions.py
  lib/spack/spack/util/url.py
  lib/spack/spack/version/git_ref_lookup.py
  var/spack/repos/builtin.mock/packages/build-error/package.py
  var/spack/repos/builtin.mock/packages/build-warnings/package.py
  var/spack/repos/builtin.mock/packages/printing-package/package.py
==> Running isort checks
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/test/cmd/install.py
All done! ✨ 🍰 ✨
1 file reformatted, 10 files left unchanged.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
Success: no issues found in 583 source files
  mypy checks were clean
==> spack style checks were clean
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've updated the branch with style fixes.

@markus-ferrell
Copy link
Contributor Author

@spackbot fix style

@spackbot-app
Copy link

spackbot-app bot commented Aug 14, 2023

Let me see if I can fix that for you!

@spackbot-app
Copy link

spackbot-app bot commented Aug 14, 2023

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
  lib/spack/spack/cmd/unit_test.py
  lib/spack/spack/directory_layout.py
  lib/spack/spack/test/cmd/install.py
  lib/spack/spack/test/directory_layout.py
  lib/spack/spack/test/util/util_url.py
  lib/spack/spack/test/versions.py
  lib/spack/spack/util/url.py
  lib/spack/spack/version/git_ref_lookup.py
  var/spack/repos/builtin.mock/packages/build-error/package.py
  var/spack/repos/builtin.mock/packages/build-warnings/package.py
  var/spack/repos/builtin.mock/packages/printing-package/package.py
==> Running isort checks
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/test/cmd/install.py
All done! ✨ 🍰 ✨
1 file reformatted, 10 files left unchanged.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
Success: no issues found in 583 source files
  mypy checks were clean
==> spack style checks were clean
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've updated the branch with style fixes.

Spack on Windows automation moved this from In Progress to Awaiting Review Aug 14, 2023
@scheibelp scheibelp merged commit d823037 into spack:develop Aug 14, 2023
33 of 34 checks passed
Spack on Windows automation moved this from Awaiting Review to Done Aug 14, 2023
mpokorny pushed a commit to mpokorny/spack that referenced this pull request Sep 18, 2023
* The module-level skip for tests in `cmd.install` on Windows is removed.
  A few classes of errors still persist:

  * Cdash tests are not working on Windows
  * Tests for failed installs are also not working (this will require
    investigating bugs in output redirection)
  * Environments are not yet supported on Windows

  overall though, this enables testing of most basic uses of "spack install"
* Git repositories cached for version lookups were using a layout that
  mimicked the URL as much as possible. This was useful for listing the
  cache directory and understanding what was present at a glance, but
  the paths were overly long on Windows. On all systems, the layout is
  now a single directory based on a hash of the Git URL and is shortened
  (which ensures a consistent and acceptable length, and also avoids
  special characters).
  * In particular, this removes util.url.parse_git_url and its associated
    test, which were used exclusively for generating the git cache layout
* Bootstrapping is now enabled for unit tests on Windows
@haampie
Copy link
Member

haampie commented Oct 3, 2023

@scheibelp / @markus-ferrell I'm uncomfortable with this path normalization you've introduced in lib/spack/spack/directory_layout.py.

Prefixes of installations are registered in the database. Now there's a change in how those paths are computed, also on Linux/macOS/BSD, since str(Path(...)) normalizes x/./y to x/y.

This may lead to very hard to discover issues with relocation, when the prefix is recomputed (for some reason).

Can you leave the old behavior on non-Windows? Or fix it in another way?

@johnwparent
Copy link
Contributor

@haampie FYI Markus is no longer on the project, so feel free to ping me in his place for this and all future issues related to his PRs.

I will look into alternative solutions, but if that looks like it'll be a long refactor, I'll just fall back on to the old behavior for this operation on *nix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants