Skip to content

WIP, ENH: Support relative mirror paths#13213

Closed
tylerjereddy wants to merge 3 commits intospack:developfrom
tylerjereddy:issue_12710_relative_mirror_paths
Closed

WIP, ENH: Support relative mirror paths#13213
tylerjereddy wants to merge 3 commits intospack:developfrom
tylerjereddy:issue_12710_relative_mirror_paths

Conversation

@tylerjereddy
Copy link
Copy Markdown
Contributor

@tylerjereddy tylerjereddy commented Oct 14, 2019

Fixes #12710 cc @AndrewGaspar @junghans

A number of things to discuss / review carefully here if core devs decide this is a good idea:

  • I've added a regression test for the new feature that fails before/ succeeds after, but could use some suggestions for improved mocking / not using actual downloads / artifacts (the spack testing infrastructure seems to have a bunch of things for this, but not necessarily easy to use naturally given the pytest fixture magic, etc.)
  • in the original issue, Andrew had suggested that the relative path support be relative to the yaml file location, but in practice this has worked out more naturally for me to be relative to the spack invocation / working directory, at least in part because one of the most natural ways to add a mirror to the yaml is from the spack command line invocation itself (and it is slightly confusing to think about path relative to i.e., ~/.spack/... when not working there, unless you edit the yaml file manually); so some design decisions to be made there
  • the requirement to prefix local filepaths with file:// currently remains, even for the relative filepath support, presumably for the curl feed-through
  • whatever is decided above, relative paths can be confusing so the behavior should be well-documented; even if relative file path support is not enabled in the end, detection & traceback on the attempt to add a relative path would still be useful vs. silent "failure" on the addition of something that can never be fetched

@tgamblin tgamblin self-requested a review October 14, 2019 18:51
@tgamblin tgamblin self-assigned this Oct 14, 2019
@junghans junghans requested a review from becker33 October 15, 2019 23:49
becker33
becker33 requested changes Oct 16, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@becker33 becker33 left a comment

Choose a reason for hiding this comment

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

I don't think this fixes #12710. The issue asked for a way to vendor tarballs in an environment, relative to the environment yaml file. Using the working directory is only the same as the directory of the environment for directory-based environments, not for Spack-managed environments.

One way to make this work could be to have spack environments automatically change working directory to the location of their yaml files. Do you think that's a feasible addition to this PR?

@tylerjereddy
TST: relative mirror paths
a509820 
* add a regression test that fails on
development branch for the handling
of relative mirror paths
@tylerjereddy
ENH: support relative mirror paths
f302a36 
* add support for fetching from relative
mirrors paths

* minor whitespace cleanup for flake8 check
in the cognate unit test
@tylerjereddy
MAINT: PR 13213 adjustments
da8acac 
* add documentation for Spack relative mirror path
support

* test_mirror_paths() now requires fetching to be possible
for a variety of spack working directories; this aims to enforce
usage of a single path relative to the yaml file, instead of a fragile
path relative to the spack working directory at the time of mirror addition

* adjust source code to properly handle addition of relative mirror
paths added via spack.config.set() in test_mirror_paths; all relative
paths are now adjusted such that they are relative to the yaml file
location

* test_mirror_paths() also adjusted to require handling of relative
paths prefixed with "file://" and not prefixed in that way, for consistency
with absolute path handling; adjust source code to handle this

* test_mirror_paths() was adjusted to probe the code path that directly
leverages "spack mirror add" via subprocess (no additional failures were
observed, so we may be able to forego this); same for addition of a mirror
via a custom file created in a custom scope

* test_mirror_paths() now checks for additional indicators of fetching
problems, including usage of external (http) and local (cache) sources
that would circumvent the code paths
@tylerjereddy tylerjereddy force-pushed the issue_12710_relative_mirror_paths branch from 816ceee to da8acac Compare October 22, 2019 19:36
@tylerjereddy
Copy link
Copy Markdown
Contributor Author

tylerjereddy commented Oct 22, 2019

The PR has been revised substantially based on the feedback above. See the new docs in the PR for how it currently works. I'll wait for more feedback after these changes, but I do note that:

  • the regression test uses a network connection & ugly subprocess commands & is also probably too slow
  • the way yaml_path is handled probably has to change--I doubt it would be desirable to have this as a "reserved mirror;" at the very least the name usage would have to be blocked, but probably it could be stored internally somehow instead
  • likely needs some practical testing outside of the complicated regression test
  • test_multimethod_diamond_inheritance fails when run in the full test suite, but not with spack test multimethod in isolation
  • haven't checked 2.x series compatibility yet

@tylerjereddy tylerjereddy mentioned this pull request Oct 25, 2019
Closed
@tldahlgren tldahlgren requested a review from becker33 October 31, 2019 19:33
@tylerjereddy
Copy link
Copy Markdown
Contributor Author

tylerjereddy commented Nov 5, 2019

Any feedback on the revised version of this PR?

@haampie
Copy link
Copy Markdown
Member

haampie commented Jan 17, 2023

I'm closing this now that #34891 is merged, as well as other PRs that ensures Spack uses file:// urls correctly

@haampie haampie closed this Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tgamblin tgamblin Awaiting requested review from tgamblin

@becker33 becker33 Awaiting requested review from becker33

Assignees

@tgamblin tgamblin

Labels

environments impact-medium

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support relative file paths in spack.yaml mirrors section

5 participants

@tylerjereddy @haampie @becker33 @tgamblin @alalazo