Added legacy repository for deprecated recipes#15420
Added legacy repository for deprecated recipes#15420alalazo wants to merge 3 commits intospack:developfrom
Conversation
closes spack#10411 closes spack#12941 This commit introduces a new repository where to move deprecated recipes e.g. for packages that changed build system. This will allow to benefit from base classes specialized for each build-system while still providing support for both recipes. Mpifileutils used as an example.
973a404 to
2b641cd
Compare
This comment has been minimized.
This comment has been minimized.
adamjstewart
left a comment
There was a problem hiding this comment.
I like this solution a lot. It's definitely by far the simplest solution. With this PR, the following simple cases can be handled properly:
- A package changes its build system a single time
As far as I can tell, the following more complicated cases cannot be handled with this approach:
- A package changes its build system more than twice
- A package supports multiple build systems for the same version
Based on previous discussion, 1 seems rare, and if it ever occurs, we can just fall back on the previous approach of a generic Package base class. 2 is pretty common, but I don't think we need to support that.
Additional things I would like to see (either in this PR or another):
- We need documentation for how to package something that changes build system, possibly in https://spack.readthedocs.io/en/latest/build_systems.html
- There are a lot of other packages that should be updated with this new logic: #12941 (comment)
This one in my opinion should be solved with a rotation. The new build system in
Here I would just use the build system that upstream developers recommend. Usually supporting two build systems at the same time happens during transition phases from one to the other, but I didn't investigate if we have less trivial cases of e.g. different build systems used for different architectures. I think though this will be a very small fraction of the supported packages. |
|
This is really clean solution! Echoing @adamjstewart 's comment, there needs to be some documentation around how to leverage this feature (it appears to be based solely on version numbers). Thanks for converting the mpifileutils package... looks great to me! |
ChristianTackeGSI
left a comment
There was a problem hiding this comment.
What about having multiple classes in the same file?
Basicly those two classes are currently just scattered around in two repos.
I don't know, what a good naming convention for "older" package classes would be, but it should be doable. That way, even (2) can be handled.
I think the approach you are suggesting requires extensive modifications of Spack. Say I have 2 classes in $ spack install hdf5if the versions overlap? How can a user specify the build-system? Can different build-systems have different variants if the classes are in the same Adding a new repository I can give a clear answer to each of the questions above and there are no core modifications to Spack, so I don't see what would be the gain following your approach. |
|
@ChristianTackeGSI As a further note (2) is doable also with this PR even though whether it's a good idea or not to maintain both build systems is debatable. We just need to maintain a build-system (the "new" one) in |
My fault. I meant (1): More than two build systems.
One possible solution would be This whole suggestion was a brainstormy idea. |
scheibelp
left a comment
There was a problem hiding this comment.
My thoughts on the overall idea:
- First inclination: multiple package support is an example of why packages should use composition rather than inheritance to get the functionality they need
- The primary issue is that packages with fixes (e.g. patches) that span the lifetimes of multiple build systems will fall prey to all the issues that plague duplicate code
- This is unlikely to affect packages currently in Spack (since most of them transitioned long ago). It is more likely to be an issue for future code projects that switch build systems. In other words, fixes that span build system lifetimes are most likely to come up near the time of a build system transition.
- That being said, the changes here don’t preclude a composition-based approach to multiple-package support
- One other problem with this approach: If I start with the configs offered in this PR, and a user calls “spack spec legacy-spec@oldversion (where oldversion appears only in the older spec), will Spack understand that it needs to use the legacy package? I don’t think it will - I’m assuming I would need to say “spack spec legacy.legacy-spec@oldversion” to avoid a complaint
I have looked through all the individual package changes and see no issues.
|
Can we make a final decision about this sometime in the near future? I want to update the |
I fully agree that in general composition is better than inheritance for code reuse. On the other hand, changing how we treat build-systems is a quite impacting design choice that deals with at least:
This solution instead:
Fyi, I have written down a few considerations about the issue of packages that change build system in #10411 (comment) My personal point of view here is that having a legacy repository can be of use also if we have composition of build-systems already implemented - for instance as a way to notify users that some packages might be removed in future releases.
I consider this another manifestation of the shortcomings of the current concretizer, in the same way as I can't specify |
Agreed. But I want to make sure we know what a legacy repo is and isn't good for: e.g. I'm concerned that code duplication will make it unsuitable for most cases which is why I'd like to consider how a composition-based approach would work. If it is decided that it is too much work and that there are use cases which need to be supported on a shorter time frame, then that would make the approach of this PR appealing. I want to avoid suddenly being responsible for condensing a number of legacy packages should it be decided that this wasn't the appropriate design decision in the future.
Each use case regarded as work for the concretizer should be recorded as such: many of these things we'd like to solve could possibly be solved, but if we don't account for it in the design, then most likely it will be difficult to support. I'd like to discuss how to avoid shifting this responsibility on the concretizer in the future. I'm thinking there could be a To address some concerns more directly:
|
|
@alalazo there's one thing I would like to understand about your proposed solution. Let's say I have a package
Spack can already handle installing versions that are not available in a package, so I want to know how these things will work if we use 2 repos. If the above use cases aren't handled correctly, then we're essentially punting the problem onto the user to explicitly specify |
Sure, we're on the same page here. There are many proposed solutions for packages that change build-systems - none of them 100% satisfactory, including this one. What I want to point out is that this solution is the simplest by far that permits to:
Code duplication is mainly for things that shouldn't be there in the first place (patches to some point release when the rule should be pristine sources) or for things that don't require a lot of maintenance (docstrings for descriptions). Also, my impression is that code in the legacy repository will be mostly "stable" in the sense that active development for the package will be done in the builtin repository - but of course I have no metric or history to support that.
I can file an issue if this is not tracked already. Just to be clear, this is a problem we currently have when adding a new repository, since this PR does not change at all the behavior of repositories or of the concretizer.
Sure. Should we continue here or should we have the discussion in a dedicated issue?
To better specify in case it was unclear, I mean the package DSL not specs. Semantically there are directives that are tied to the build-system. For instance, |
|
@adamjstewart To reply to your questions:
Currently only 2 and 3 and you need to say
Currently you need to specify
Same behavior as
This I didn't try but as above there's no change in behavior in this PR with respect to
I kind of disagree:
|
|
@adamjstewart @scheibelp Added #15800 to keep track of this use case for concretization. |
There would be an error: To handle this, the user would have to specify |
|
@scheibelp why would that be an error? Spack already allows you to install versions of packages that aren't listed, so I would imagine it would confirm whether or not you want to download that version even though there is no checksum available, then install it via CMake. |
I think that's true for all current packages in Spack. I think it won't be true for any package in the future which decides to switch build systems: it's common for developers to support the last few recent versions, and in their case that would include versions with different build systems.
The concretizer this far has never been expected to select among repositories for the most suitable package (not in the same way that it chooses among providers for the best virtual) - that's an entirely-new concept and I think this PR is the first place that I've seen where the concretizer has been expected to do that.
I agree that patches will always ideally disappear at some point, but I don't think this means anything in terms of Package implementation: the code still has to be there, and if users forget to duplicate their changes it still breaks.
I acknowledge that this is the simplest solution. I want to explore the other criteria though (e.g. whether it handles the use cases that people need). For example based on #15420 (comment) and #15420 (comment), we may start to see packages in the non-legacy repo that reference legacy packages. Also, perhaps we can come up with an approach that addresses versions with overlapping build systems that you refer to in #10411 (comment). This solution may be good for a subset of legacy packages, but I one thing I want to come out of this conversation is the guidelines for which packages should not be turned into legacy packages. Spack package reviewers will have to be aware of these constraints when looking at new PRs, otherwise the relationship between the legacy/non-legacy repos may become over-complicated. |
Oh you are correct. I would consider that also to be an error, but you are right that it would be Spack doing something different vs. complaining. |
What I've seen so far is that packages switch build-system from one version to the next, or in any case very quickly: see #15812 for a recent example. The only example I have of a package that maintains 2 build-systems is A case I never encountered but that is possible is that a similar package might decide to keep |
closes #10411
closes #12941
This commit introduces a new repository where to move deprecated recipes e.g. for packages that changed build system. This will allow to benefit from base classes specialized for each build-system while still providing support for both recipes.
Legacy packages used as example: