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.

Sign up for GitHub

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

Jump to bottom

fix spack load --list and spack unload on windows #35720

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

fix spack load --list and spack unload on windows #35720

wants to merge 7 commits into from

Conversation

danlipsa
Copy link
Contributor

@danlipsa danlipsa commented Feb 27, 2023 •
edited

fixes for:
spack load --list (this printed 0 packages even if packages were loaded)
spack unload 'package' (this said that the package is not loaded even if it was).
spack external find -p "c:\Program Files\CMake-3.24.2" cmake (this gave an error caused by multiple quotes inside a string used for comparison)

@spackbot-app spackbot-app bot added commands core PR affects Spack core functionality labels Feb 27, 2023
@danlipsa danlipsa changed the title Spack load list windows fix spack load --list on windows Feb 27, 2023
@danlipsa
Copy link
Contributor Author

@johnwparent Please review

@danlipsa danlipsa changed the title fix spack load --list on windows fix spack load --list and spack unload on windows Feb 27, 2023
@danlipsa
Copy link
Contributor Author

@scheibelp Please review

@scheibelp
Copy link
Member

A couple general requests:

  • I assume there are associated (but currently disabled on Windows) tests for spack load functionality: ideally they would be reenabled as part of this PR. Does that seem reasonable?
  • Can you update the PR description with the what and why?

@danlipsa
Copy link
Contributor Author

danlipsa commented Mar 1, 2023

  • I assume there are associated (but currently disabled on Windows) tests for spack load functionality: ideally they would be reenabled as part of this PR. Does that seem reasonable?
    @scheibelp That would be great! Can you point me to some documentation on how to run the tests or give me some brief pointers. Thanks!
  • Can you update the PR description with the what and why?
    How does the updated description look?

@scheibelp
Copy link
Member

I assume there are associated (but currently disabled on Windows) tests for spack load functionality: ideally they would be reenabled as part of this PR. Does that seem reasonable?

@scheibelp That would be great! Can you point me to some documentation on how to run the tests or give me some brief pointers. Thanks!

test/cmd/load.py is the one I see (there is a module level disabling of tests for Windows).

Can you update the PR description with the what and why?

How does the updated description look?

Good: thanks!

@scheibelp scheibelp added this to In Progress in Spack on Windows via automation Mar 4, 2023
@johnwparent johnwparent moved this from In Progress to Awaiting Review in Spack on Windows Mar 13, 2023
@spackbot-app spackbot-app bot added the tests General test capability(ies) label Mar 13, 2023
@danlipsa
Copy link
Contributor Author

danlipsa commented Mar 14, 2023 •
edited

@scheibelp I enabled/updated the tests in load.py and a two tests in find.py and now they run successfully on windows. Please review.

@danlipsa
Copy link
Contributor Author

@johnwparent I rebased this PR over devel and the load.py tests still pass on my machine.

@johnwparent
Copy link
Contributor

Thanks for the upkeep Dan! Changes in general still look good to me, so pending CI passing, this is ready for another round of review.

@danlipsa
Copy link
Contributor Author

danlipsa commented Jun 7, 2023

@scheibelp This branch is also ready.

@danlipsa danlipsa force-pushed the spack_load_list_windows branch 2 times, most recently from ed819ec to acfd8ea Compare June 28, 2023 13:14
@danlipsa
Copy link
Contributor Author

@scheibelp ping. This passes all tests

scheibelp
scheibelp requested changes Jun 30, 2023
View reviewed changes
bin/spack.bat Outdated Show resolved Hide resolved
lib/spack/spack/cmd/unit_test.py Outdated Show resolved Hide resolved
lib/spack/spack/test/cmd/load.py Outdated Show resolved Hide resolved
lib/spack/spack/test/cmd/load.py Outdated Show resolved Hide resolved
Spack on Windows automation moved this from Awaiting Review to In Progress Jun 30, 2023
@danlipsa danlipsa force-pushed the spack_load_list_windows branch 4 times, most recently from 794f00e to e4bd0b9 Compare July 10, 2023 17:13
@danlipsa
Copy link
Contributor Author

@scheibelp Please review. Thanks!

@danlipsa danlipsa force-pushed the spack_load_list_windows branch 2 times, most recently from 285b7b9 to 484da44 Compare September 8, 2023 18:21
@danlipsa danlipsa force-pushed the spack_load_list_windows branch 3 times, most recently from 6d4c34e to d371fea Compare May 7, 2024 22:36
@danlipsa
Copy link
Contributor Author

danlipsa commented May 8, 2024

@johnwparent @scheibelp Please review. I rebased this PR on develop - all the tests in load.py pass on windows now.

johnwparent
johnwparent reviewed May 21, 2024
View reviewed changes
bin/spack.bat Outdated Show resolved Hide resolved
bin/spack.bat
goto :default_case
)
if NOT defined _sp_args (
python "%spack%" "%_sp_subcommand%" --help
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the behavior I observe on linux when spack load is invoked with no arguments. There, the command seems to exit silently without invoking the help. Seems like these should be the same (and IMO this approach is better)
@scheibelp thoughts?

bin/spack.bat Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnwparent johnwparent johnwparent left review comments

@scheibelp scheibelp scheibelp requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@scheibelp scheibelp

Labels
commands core PR affects Spack core functionality tests General test capability(ies) utilities
Projects
Spack on Windows
In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@danlipsa @scheibelp @johnwparent