Refer to mirrors by name, path, or url#34891
Merged
tgamblin merged 2 commits intospack:developfrom Jan 16, 2023
Merged
Conversation
spackbot-app
bot
added
commands
core
haampie
commented
Jan 11, 2023
haampie
force-pushed
the
fix/mirror-path-url-2
branch
3 times, most recently
from
9f87a9c to
a6a6eb8
Compare
haampie
commented
Jan 11, 2023
| """Add a mirror to Spack.""" | ||
| url = url_util.format(args.url) | ||
| spack.mirror.add(args.name, url, args.scope, args) | ||
| spack.mirror.add(args.name, args.url, args.scope, args) |
Member
Author
There was a problem hiding this comment.
@tgamblin so, I'm just adding mirror urls/paths verbatim now, what do you think? That way you can do spack -e . mirror add my_mirror '$env/../mirror' properly. Expanding/canonicalizing/urlifying is done when mirror_instance.fetch_url is retrieved whenever mirrors are actually used.
Member
There was a problem hiding this comment.
I like this because it goes along with respecting the user said -- either on the CLI or in a YAML file. I guess the only only question I have is what happens if the user says:
spack mirror add ./foo
When the scope to add ./foo in is an environment but the user is not in the env directory? I am not 100% sure whether in that case we should be absolutifying the path. I wonder how much this comes up.
It looks like git does what you do and stores the path verbatim. Here's an example in a directory that is not alongside my spack installation in ~/src/spack:
(spackbook):2022> git init ../foobar
Initialized empty Git repository in /Users/gamblin2/pubs/presentations/foobar/.git/
(spackbook):2022> git -C ~/src/spack remote add blah ../foobar
(spackbook):2022> git -C ~/src/spack remote -v|grep foobar
blah ../foobar (fetch)
blah ../foobar (push)(spackbook):2022> git -C ~/src/spack pull blah
fatal: '../foobar' does not appear to be a git repository
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.(spackbook):2022> mv ../foobar ~/src
(spackbook):2022> git -C ~/src/spack pull blah
You asked to pull from the remote 'blah', but did not specify
a branch. Because this is not the default configured remote
for your current branch, you must specify a branch on the command line.So I think what you're doing is fine -- I know I've never actually noticed this issue for git so I doubt I would notice it for spack. I suspect people using relative paths will be trying to do for some special environment.
| SPACK_COMPREPLY="-h --help -r --rel -f --force -u --unsigned -a --allow-root -k --key -d --directory -m --mirror-name --mirror-url --rebuild-index --spec-file --only" | ||
| else | ||
| _all_packages | ||
| _mirrors |
Member
Author
There was a problem hiding this comment.
This is a bit sad, but nothing I can do about it :(
haampie
force-pushed
the
fix/mirror-path-url-2
branch
from
a6a6eb8 to
6dd7355
Compare
haampie
force-pushed
the
fix/mirror-path-url-2
branch
2 times, most recently
from
43c44f0 to
e903efb
Compare
With this change we get that `mirror.fetch_url` and `mirror.push_url` always return valid URLs, even when the config is actually using (relative) paths. Also change the interface so that we can now do e.g. ``` spack buildache create [mirror] [specs...] ``` where `mirror` is either a mirror name, a path, or a URL. Resolving the relevant mirror goes as follows: - If it contains either / or \ it is used as an anonymous mirror with path or url. - Otherwise, it's interpreted as a named mirror, which must exist. This guards against typos in mirror names (e.g. avoid pushing to a local dir), and it's easy to validate without hitting the filesystem: ``` $ spack -e . buildcache create my-mirror ==> Error: no mirror named "my-mirror". Did you mean ./my-mirror? ``` It's up to the `[fetch/push]_url` properties to distinguish between URLs and paths. Right now it's considered a URL when the scheme matches something that is supported, otherwise it's a path. When it is a path, we further use canonicalization, and transform to a file URL.
haampie
force-pushed
the
fix/mirror-path-url-2
branch
from
e903efb to
8648f83
Compare
spackbot-app
bot
added
the
documentation
Member
|
@blue42u FYI |
3 tasks
Member
Author
Contributor
|
I like the direction this is going, thanks! Would this be easy to extend to |
Member
Author
|
Can we consider merging this @tgamblin? |
tgamblin
requested changes
Jan 16, 2023
tgamblin
approved these changes
Jan 16, 2023
This was referenced Jan 17, 2023
RikkiButler20
pushed a commit
to RikkiButler20/spack
that referenced
this pull request
Jan 24, 2023
With this change we get the invariant that `mirror.fetch_url` and
`mirror.push_url` return valid URLs, even when the backing config
file is actually using (relative) paths with potentially `$spack` and
`$env` like variables.
Secondly it avoids expanding mirror path / URLs too early,
so if I say `spack mirror add name ./path`, it stays `./path` in my
config. When it's retrieved through MirrorCollection() we
exand it to say `file://<env dir>/path` if `./path` was set in an
environment scope.
Thirdly, the interface is simplified for the relevant buildcache
commands, so it's more like `git push`:
```
spack buildcache create [mirror] [specs...]
```
`mirror` is either a mirror name, a path, or a URL.
Resolving the relevant mirror goes as follows:
- If it contains either / or \ it is used as an anonymous mirror with
path or url.
- Otherwise, it's interpreted as a named mirror, which must exist.
This helps to guard against typos, e.g. typing `my-mirror` when there
is no such named mirror now errors with:
```
$ spack -e . buildcache create my-mirror
==> Error: no mirror named "my-mirror". Did you mean ./my-mirror?
```
instead of creating a directory in the current working directory. I
think this is reasonable, as the alternative (requiring that a local dir
exists) feels a bit pendantic in the general case -- spack is happy to
create the build cache dir when needed, saving a `mkdir`.
The old (now deprecated) format will still be available in Spack 0.20,
but is scheduled to be removed in 0.21:
```
spack buildcache create (--directory | --mirror-url | --mirror-name) [specs...]
```
This PR also touches `tmp_scope` in tests, because it didn't really
work for me, since spack fixes the possible --scope values once and
for all across tests, so tests failed when run out of order.
nhanford
pushed a commit
to nhanford/spack
that referenced
this pull request
Feb 15, 2023
With this change we get the invariant that `mirror.fetch_url` and
`mirror.push_url` return valid URLs, even when the backing config
file is actually using (relative) paths with potentially `$spack` and
`$env` like variables.
Secondly it avoids expanding mirror path / URLs too early,
so if I say `spack mirror add name ./path`, it stays `./path` in my
config. When it's retrieved through MirrorCollection() we
exand it to say `file://<env dir>/path` if `./path` was set in an
environment scope.
Thirdly, the interface is simplified for the relevant buildcache
commands, so it's more like `git push`:
```
spack buildcache create [mirror] [specs...]
```
`mirror` is either a mirror name, a path, or a URL.
Resolving the relevant mirror goes as follows:
- If it contains either / or \ it is used as an anonymous mirror with
path or url.
- Otherwise, it's interpreted as a named mirror, which must exist.
This helps to guard against typos, e.g. typing `my-mirror` when there
is no such named mirror now errors with:
```
$ spack -e . buildcache create my-mirror
==> Error: no mirror named "my-mirror". Did you mean ./my-mirror?
```
instead of creating a directory in the current working directory. I
think this is reasonable, as the alternative (requiring that a local dir
exists) feels a bit pendantic in the general case -- spack is happy to
create the build cache dir when needed, saving a `mkdir`.
The old (now deprecated) format will still be available in Spack 0.20,
but is scheduled to be removed in 0.21:
```
spack buildcache create (--directory | --mirror-url | --mirror-name) [specs...]
```
This PR also touches `tmp_scope` in tests, because it didn't really
work for me, since spack fixes the possible --scope values once and
for all across tests, so tests failed when run out of order.
amd-toolchain-support
pushed a commit
to amd-toolchain-support/spack
that referenced
this pull request
Feb 16, 2023
With this change we get the invariant that `mirror.fetch_url` and
`mirror.push_url` return valid URLs, even when the backing config
file is actually using (relative) paths with potentially `$spack` and
`$env` like variables.
Secondly it avoids expanding mirror path / URLs too early,
so if I say `spack mirror add name ./path`, it stays `./path` in my
config. When it's retrieved through MirrorCollection() we
exand it to say `file://<env dir>/path` if `./path` was set in an
environment scope.
Thirdly, the interface is simplified for the relevant buildcache
commands, so it's more like `git push`:
```
spack buildcache create [mirror] [specs...]
```
`mirror` is either a mirror name, a path, or a URL.
Resolving the relevant mirror goes as follows:
- If it contains either / or \ it is used as an anonymous mirror with
path or url.
- Otherwise, it's interpreted as a named mirror, which must exist.
This helps to guard against typos, e.g. typing `my-mirror` when there
is no such named mirror now errors with:
```
$ spack -e . buildcache create my-mirror
==> Error: no mirror named "my-mirror". Did you mean ./my-mirror?
```
instead of creating a directory in the current working directory. I
think this is reasonable, as the alternative (requiring that a local dir
exists) feels a bit pendantic in the general case -- spack is happy to
create the build cache dir when needed, saving a `mkdir`.
The old (now deprecated) format will still be available in Spack 0.20,
but is scheduled to be removed in 0.21:
```
spack buildcache create (--directory | --mirror-url | --mirror-name) [specs...]
```
This PR also touches `tmp_scope` in tests, because it didn't really
work for me, since spack fixes the possible --scope values once and
for all across tests, so tests failed when run out of order.
techxdave
pushed a commit
to Tech-XCorp/spack
that referenced
this pull request
Feb 17, 2023
With this change we get the invariant that `mirror.fetch_url` and
`mirror.push_url` return valid URLs, even when the backing config
file is actually using (relative) paths with potentially `$spack` and
`$env` like variables.
Secondly it avoids expanding mirror path / URLs too early,
so if I say `spack mirror add name ./path`, it stays `./path` in my
config. When it's retrieved through MirrorCollection() we
exand it to say `file://<env dir>/path` if `./path` was set in an
environment scope.
Thirdly, the interface is simplified for the relevant buildcache
commands, so it's more like `git push`:
```
spack buildcache create [mirror] [specs...]
```
`mirror` is either a mirror name, a path, or a URL.
Resolving the relevant mirror goes as follows:
- If it contains either / or \ it is used as an anonymous mirror with
path or url.
- Otherwise, it's interpreted as a named mirror, which must exist.
This helps to guard against typos, e.g. typing `my-mirror` when there
is no such named mirror now errors with:
```
$ spack -e . buildcache create my-mirror
==> Error: no mirror named "my-mirror". Did you mean ./my-mirror?
```
instead of creating a directory in the current working directory. I
think this is reasonable, as the alternative (requiring that a local dir
exists) feels a bit pendantic in the general case -- spack is happy to
create the build cache dir when needed, saving a `mkdir`.
The old (now deprecated) format will still be available in Spack 0.20,
but is scheduled to be removed in 0.21:
```
spack buildcache create (--directory | --mirror-url | --mirror-name) [specs...]
```
This PR also touches `tmp_scope` in tests, because it didn't really
work for me, since spack fixes the possible --scope values once and
for all across tests, so tests failed when run out of order.
jmcarcell
pushed a commit
to key4hep/spack
that referenced
this pull request
Apr 13, 2023
With this change we get the invariant that `mirror.fetch_url` and
`mirror.push_url` return valid URLs, even when the backing config
file is actually using (relative) paths with potentially `$spack` and
`$env` like variables.
Secondly it avoids expanding mirror path / URLs too early,
so if I say `spack mirror add name ./path`, it stays `./path` in my
config. When it's retrieved through MirrorCollection() we
exand it to say `file://<env dir>/path` if `./path` was set in an
environment scope.
Thirdly, the interface is simplified for the relevant buildcache
commands, so it's more like `git push`:
```
spack buildcache create [mirror] [specs...]
```
`mirror` is either a mirror name, a path, or a URL.
Resolving the relevant mirror goes as follows:
- If it contains either / or \ it is used as an anonymous mirror with
path or url.
- Otherwise, it's interpreted as a named mirror, which must exist.
This helps to guard against typos, e.g. typing `my-mirror` when there
is no such named mirror now errors with:
```
$ spack -e . buildcache create my-mirror
==> Error: no mirror named "my-mirror". Did you mean ./my-mirror?
```
instead of creating a directory in the current working directory. I
think this is reasonable, as the alternative (requiring that a local dir
exists) feels a bit pendantic in the general case -- spack is happy to
create the build cache dir when needed, saving a `mkdir`.
The old (now deprecated) format will still be available in Spack 0.20,
but is scheduled to be removed in 0.21:
```
spack buildcache create (--directory | --mirror-url | --mirror-name) [specs...]
```
This PR also touches `tmp_scope` in tests, because it didn't really
work for me, since spack fixes the possible --scope values once and
for all across tests, so tests failed when run out of order.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With this change we get the invariant that
mirror.fetch_urlandmirror.push_urlreturn valid URLs, even when the backing configfile is actually using (relative) paths with potentially
$spackand$envlike variables.Secondly it avoids expanding mirror path / URLs too early,
so if I say
spack mirror add name ./path, it stays./pathin myconfig. When it's retrieved through MirrorCollection() we
exand it to say
file://<env dir>/pathif./pathwas set in anenvironment scope.
Thirdly, the interface is simplified for the relevant buildcache
commands, so it's more like
git push:mirroris either a mirror name, a path, or a URL.Resolving the relevant mirror goes as follows:
path or url.
This helps to guard against typos, e.g. typing
my-mirrorwhen thereis no such named mirror now errors with:
instead of creating a directory in the current working directory. I
think this is reasonable, as the alternative (requiring that a local dir
exists) feels a bit pendantic in the general case -- spack is happy to
create the build cache dir when needed, saving a
mkdir.The old (now deprecated) format will still be available in Spack 0.20,
but is scheduled to be removed in 0.21:
This PR also touches
tmp_scopein tests, because it didn't reallywork for me, since spack fixes the possible --scope values once and
for all across tests, so tests failed when run out of order.