-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Allow packages without URLs #3161
Conversation
I have no idea... Can you try it out for a while and see what happens? If it breaks, the break should be pretty obvious (missing attribute). |
Another alternative would be to initialize |
Chiming in to say this fixed the issue I ran into with #8745. I'll keep this commit cherry-picked to see if it causes any issues. What else needs to be done to get this PR merged? |
Mostly testing by people like you so that you can let us know if it breaks anything unexpected down the line. And a review from someone who knows more about Spack's internals than I do. |
I’ve got some additions and tests I’ll push soon. I think it’ll be ready then. |
aaeb4e6
to
4df4193
Compare
@adamjstewart: I added some more machinery and more tests, and I think you are now in the odd position of needing to review your own PR. The details are in the commit messages but here they are for folks on this PR:
This probably fixes a few more issues than are listed in the main PR, as it should make top-level Also, while this keeps the policy for extrapolation we decided on in #8565 (trust the package-level URL first), it also brings back the |
I've been wanting this for the longest time! One step closer to the interface I envisioned in #2718. I'll try to reproduce all of the bugs listed above and see if this does indeed fix them. If I don't get to this tomorrow, I'll definitely get to it by the weekend. I would be interested in removing fake URLs and adding top-level |
@adamjstewart: I think the interface you proposed there will work now. If it doesn't, add a test package for what it should look like and I don't think it would be too much more effort to make it work. |
@HadrienG2: FYI |
I think this is up to you. It sounds good either way! Also if you don't mind adding docs for it, that would be awesome. |
I would lean toward another PR. My two cents...
…On Thu, Jul 19, 2018 at 12:23 AM Todd Gamblin ***@***.***> wrote:
I would be interested in removing fake URLs and adding top-level git, hg,
and svn attributes to dozens of packages in Spack. Should I add that to
this PR or wait until after this is merged?
I think this is up to you. It sounds good either way! Also if you don't
mind adding docs for it, that would be awesome.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3161 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AB1cd6EaHg5LCNeXHQB3qaBCCCZIOd_Eks5uIAmigaJpZM4MDcd2>
.
|
I can definitely add docs. If I make the package changes in this PR, it becomes easier to test that this PR actually works and none of the behavior will change. |
P.S. Still dreaming of the day when #1983
<#1983> is merged and we no longer
have to tell people to rename their master branch to develop:
I would love to have something more functional too, but I think that #1983
has some serious shortcomings. I would love a process by which all parties
involved can hammer out a plan that we can all support.
…-- Elizbaeth
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, docs have now been updated. I think this PR is ready for a final review as far as I'm concerned!
P.S. I'm moving to a new aprt this weekend, so I'm going to be somewhat busy (un)packing. I'll be able to respond to comments and make small changes, but if you need to make any big changes, feel free to push directly onto this PR. I'm hoping to get this merged quickly as it affects so many packages.
|
||
^^^^^^^^^^^^^^^^^^^^^ | ||
PyPI and version URLs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section has been moved to http://spack.readthedocs.io/en/latest/build_systems/pythonpackage.html#url.
levels of subdirectories, as the ``mpich`` website is structured much | ||
``list_url``. ``list_depth = 1`` tells it to follow up to 1 level of | ||
links from the top-level page. Note that here, this implies 1 | ||
level of subdirectories, as the ``mpich`` website is structured much |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this was a bug in list_depth
that @tgamblin fixed a long time ago but the docs were never updated.
there is no way to verify that the repository has not been compromised, and | ||
the commit you get when you install the package likely won't be the same | ||
commit that was used when the package was first written. Additionally, the | ||
default branch may change. It is best to at least specify a branch name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to expand on each of these "This download method is untrusted, and is not recommended" statements to explain the rationale and what alternatives are available. I think this is sufficient to close #6785.
.. _go-fetch: | ||
|
||
^^ | ||
Go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently Spack supports fetching with Go? Well, it's documented now! Tbh, I don't know the first thing about Go, so if this section feels... wishy-washy, that's why. Someone should definitely take a close look at this section and correct me wherever I'm wrong.
Some packages do not have a `url` and are instead downloaded via `git`, `hg`, or `svn`. Some packages like `spectrum-mpi` cannot be downloaded at all, and are placeholder packages for system installations. Previously, `__init__()` in `PackageBase` crashed if a package did not have a `url` attribute defined. I hacked this section of code out, but I have no idea what the repercussions of that are.
- Packages can remove the top-level `url` attribute and still work - These are now legal: - Packages with *only* version-specific URLs (even with gaps) - Packages with a top-level git/hg/svn attribute and `version` directives for that. - If a package has both a top-level hg/git/svn attribute AND a top-level url attribute, the url attribute takes precedence.
- ensure that packages can't have more than one of git, hg, svn, or url
- packagers can specify two top-level fetch URLs if one is `url` - e.g., `url` and `git` or `url` and `svn` - allow only one VCS fetcher so we can differentiate between URL and VCS. - also clean up fetcher logic and class structure
- this test is checking the package *repository*, not the database.
- Previously, Spack didn't check the arguments you put in version() directives. - So, you could do something like this, where there are arguments for a URL fetcher AND for a git fetcher: version('1.0', md5='abc123', git='https://foo.bar', commit='feda2343') - Now, we check the arguments before constructing a fetcher, to ensure that each package has *only* arguments for a single type of fetcher. - Also added `test_package_version_consistency()` to the `package_sanity` test, so that all builtin packages are required to have valid `version()` directives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamjstewart: I read through the docs. I think they're good enough for now. There are some clarifications we should make to the wording ("trusted" vs. verifiable, etc.), and a few things we should add to the fetch strategy to satisfy some of @HadrienG2's requests, but I think those belong in another PR. The issues were there before.
To me, this looks great -- thanks for fixing and documenting everything!
looks like this was just a cli usage error: #2788 (comment) |
Fixes #98
Fixes #3042
Fixes #7971
Fixes #8046
Fixes #8173
Fixes #8733
Fixes #8745
Closes #6785
Some packages do not have a
url
and are instead downloaded viagit
,hg
, orsvn
. Some packages likespectrum-mpi
cannot be downloaded at all, and are placeholder packages for system installations. Previously,__init__()
inPackageBase
crashed if a package did not have aurl
attribute defined.I hacked this section of code out, but I have no idea what the repercussions of that are. Personally, I don't know why we would want to call
url_for_version
during package creation at all. This change probably breaks something, so let me know if you can think of anything that required this.@tgamblin @citibeth