Skip to content

Fixes to Handling Multiple Architectures#2261

Merged
tgamblin merged 13 commits intospack:developfrom
xjrc:bugfix/multiarch-detection
Dec 3, 2016
Merged

Fixes to Handling Multiple Architectures#2261
tgamblin merged 13 commits intospack:developfrom
xjrc:bugfix/multiarch-detection

Conversation

@xjrc
Copy link
Copy Markdown
Member

@xjrc xjrc commented Nov 6, 2016

Fixes #2249. Fixes #1815.

The changes in this pull request will fix the issues described in issue #2249 and generally improve architecture handling in Spack. These changes include the following updates:

  • Implement the spack.spec.ArchSpec type to represent abstract architecture requirements (e.g. like spack.spec.CompilerSpec does for compilers).
  • Replace the architecture information being used in the spack.spec.Spec type with spack.spec.ArchSpec to facilitate better ambiguous architecture handling.
  • Add unit tests to verify that the functionality specific to spack.spec.ArchSpec works properly.

@xjrc xjrc added bug Something isn't working WIP labels Nov 6, 2016
@xjrc xjrc self-assigned this Nov 6, 2016
@xjrc
Copy link
Copy Markdown
Member Author

xjrc commented Nov 6, 2016

@tgamblin: After looking over the spack.architecture code a bit, it looks like the problems related to multi-architecture handling stem from the fact that all new architectures are constructing using the current host architecture as a base. I can think of two ways to fix this:

  1. Store all of the information about the host architecture (instead of just the platform, platform os, and target) in the Spack database when installing packages and then use this information to create new architecture objects when reading specs from the database.
  2. Continue using the host architecture as the base and simply update the operating system and target of architecture object created for each package based on its associated platform_os and target database fields (which assumes that the host platform is the same as that used for all installed packages).

The first method is more general, but requires updating the database fields for the benefit of allowing a single Spack instance to handle installs that span different platforms (which I'm not sure is something that would be done very often if at all). With that being said, which of these methods seems preferable to you (or do you have a better alternative in mind)? Please let me know whenever you get a chance!

@tgamblin
Copy link
Copy Markdown
Member

tgamblin commented Nov 7, 2016

@xjrc: I don't think we need to be as general as (1), as the platform metadata isn't needed unless you are actually building on the host. The way we've separated things out for other parts of Spack, like compilers, is to have a CompilerSpec object that's just a descriptor, then a separate Compiler object that you can look up using the Spec. The Compiler objects are dependent on things like local compiler paths, but that is (intentionally) not stored in the DB -- only build provenance is stored in the DB and Specs describe the DAG and contain build provenance.

So the way I would do this is to pull an ArchSpec class out of the current Arch class, and move all the Spec methods (satisfies, to_dict, etc.) over to ArchSpec. That way Spec i just a descriptor that can live wherever. The other stuff -- like making an actual platform object, establishing the default OS for this platform, picking a target, etc, can stay where it is, and can be lazily evaluated when needed -- does that make sense?

@xjrc
Copy link
Copy Markdown
Member Author

xjrc commented Nov 7, 2016

@tgamblin: The concept makes sense, and I can certainly envision how this could be used to replace the current functionality for generating architecture information from the Spack database. I'm not entirely sure how to go about implementing ArchSpec, but I think that looking a bit more at how CompilerSpec is implemented and how it's used with Compiler will give me some ideas. Thanks!

@xjrc
Copy link
Copy Markdown
Member Author

xjrc commented Nov 8, 2016

@tgamblin: I finished implementing a preliminary version of the spack.spec.ArchSpec class. Would you mind taking a quick look at it and letting me know whether or not I'm on the right track? I just want to make sure I'm not doing anything too unusual before I start integrating this into the spack.spec.Spec class and other parts of the code.

@xjrc xjrc force-pushed the bugfix/multiarch-detection branch from e54e7c5 to 41b8782 Compare November 10, 2016 20:06
@tgamblin
Copy link
Copy Markdown
Member

@xjrc: this is actually looking really good. The fact that it also adapts the compilers could also fix another current problem: the compilers can't currently differentiate from each other by target. We see this on the ppc64le systems when they share a home dir with Linux machines.

I will take a more detailed look tonight, but it looks close to merge-able to me.

@xjrc xjrc added ready and removed WIP labels Nov 10, 2016
@becker33
Copy link
Copy Markdown
Member

@xjrc Can you check the box that lets us push to your PR?

@xjrc
Copy link
Copy Markdown
Member Author

xjrc commented Nov 28, 2016

@becker33: It looks to me like the box is already checked. Are you having problems pushing?

@becker33
Copy link
Copy Markdown
Member

@xjrc I get 403 Forbidden error when attempting to push to this branch, I would assume that's from that setting being messed up but I'm sure it could come from something else.

@tgamblin
Copy link
Copy Markdown
Member

@becker33: you need to use the git URL for the remote to be able to authenticate with ssh and push to branches like this one.

@becker33
Copy link
Copy Markdown
Member

@tgamblin I only switched to https because I got a similar fatal error when trying to push to the same branch through ssh.

@becker33 becker33 force-pushed the bugfix/multiarch-detection branch from 5788cd0 to abd39ab Compare November 29, 2016 19:27
@becker33
Copy link
Copy Markdown
Member

Pushing issues resolved. I'll resolve the merge conflicts.

@becker33 becker33 force-pushed the bugfix/multiarch-detection branch from abd39ab to 09881e7 Compare November 29, 2016 23:17
@becker33 becker33 closed this Nov 30, 2016
@becker33 becker33 reopened this Nov 30, 2016
@xjrc
Copy link
Copy Markdown
Member Author

xjrc commented Dec 2, 2016

@tgamblin, @becker33: Are there any other adjustments that need to be made to this PR before it can be merged? If so, I'd be happy to help out if I can!

xjrc and others added 6 commits December 3, 2016 14:44
Removed a number of unused architecture functions.
…ail.

Added a couple of additional regression tests related to architecture parsing/specification.
Fixed a few bugs with setting reserved os/target values on "ArchSpec" objects.
Removed a number of unnecessary functions in the "spack.architecture" and "spack.concretize" modules.
Updated the tests to use a uniform architecture to improve reliability.
Fixed a few minor style issues.
Improved error detection for multiple arch components in a spec.
@tgamblin tgamblin force-pushed the bugfix/multiarch-detection branch from 09881e7 to 414bd06 Compare December 3, 2016 23:08
@tgamblin tgamblin force-pushed the bugfix/multiarch-detection branch from 414bd06 to f0e93c6 Compare December 3, 2016 23:26
@tgamblin tgamblin merged commit 552b4ea into spack:develop Dec 3, 2016
citibeth pushed a commit to citibeth/spack that referenced this pull request Dec 4, 2016
* Added some notes about how multiarch detection could be fixed.

* Implemented a preliminary version of the "spack.spec.ArchSpec" class.

* Updated the "spack.spec.Spec" class to use "ArchSpec" instead of "Arch".

* Fixed a number of small bugs in the "spack.spec.ArchSpec" class.

* Fixed the 'Concretizer.concretize_architecture' method so that it uses the new architecture specs.

* Updated the package class to properly use arch specs.
Removed a number of unused architecture functions.

* Fixed up a number of bugs that were causing the regression tests to fail.
Added a couple of additional regression tests related to architecture parsing/specification.
Fixed a few bugs with setting reserved os/target values on "ArchSpec" objects.
Removed a number of unnecessary functions in the "spack.architecture" and "spack.concretize" modules.

* Fixed a few bugs with reading architecture information from specs.
Updated the tests to use a uniform architecture to improve reliability.
Fixed a few minor style issues.

* Adapted the compiler component of Spack to use arch specs.

* Implemented more test cases for the extended architecture spec features.
Improved error detection for multiple arch components in a spec.

* Fix for backwards compatibility with v0.8 and prior

* Changed os to unknown for compatibility specs

* Use `spack09` instead of `spackcompat` for the platform of old specs.
kserradell pushed a commit to kserradell/spack that referenced this pull request Dec 9, 2016
* Added some notes about how multiarch detection could be fixed.

* Implemented a preliminary version of the "spack.spec.ArchSpec" class.

* Updated the "spack.spec.Spec" class to use "ArchSpec" instead of "Arch".

* Fixed a number of small bugs in the "spack.spec.ArchSpec" class.

* Fixed the 'Concretizer.concretize_architecture' method so that it uses the new architecture specs.

* Updated the package class to properly use arch specs.
Removed a number of unused architecture functions.

* Fixed up a number of bugs that were causing the regression tests to fail.
Added a couple of additional regression tests related to architecture parsing/specification.
Fixed a few bugs with setting reserved os/target values on "ArchSpec" objects.
Removed a number of unnecessary functions in the "spack.architecture" and "spack.concretize" modules.

* Fixed a few bugs with reading architecture information from specs.
Updated the tests to use a uniform architecture to improve reliability.
Fixed a few minor style issues.

* Adapted the compiler component of Spack to use arch specs.

* Implemented more test cases for the extended architecture spec features.
Improved error detection for multiple arch components in a spec.

* Fix for backwards compatibility with v0.8 and prior

* Changed os to unknown for compatibility specs

* Use `spack09` instead of `spackcompat` for the platform of old specs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Spack Fails to Properly Identify Installs on Different Platforms Spack doesn't properly read OS descriptor

3 participants