Skip to content

Deprecate config:install_missing_compilers#46237

Merged
alalazo merged 5 commits intospack:developfrom
alalazo:maintainers/remove-bootstrapping-compilers
Sep 10, 2024
Merged

Deprecate config:install_missing_compilers#46237
alalazo merged 5 commits intospack:developfrom
alalazo:maintainers/remove-bootstrapping-compilers

Conversation

@alalazo
Copy link
Copy Markdown
Member

@alalazo alalazo commented Sep 5, 2024

depends on #46221

The option config:install_missing_compilers is currently buggy, and has been for a while. Remove it, since it won't be needed when compilers are treated as dependencies.

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.

There is a compiler attribute on spack.installer.BuildTask that I believe is only used to track behavior for compiler bootstrapping -- we should probably remove that attribute as well.

There is also a sequencing issue with this PR and #39136, we should think carefully about which rebase would be less unpleasant before merging these.

The option config:install_missing_compilers is currently buggy,
and has been for a while. Remove it, since it won't be needed
when compilers are treated as dependencies.

Signed-off-by: Massimiliano Culpo <[email protected]>
Signed-off-by: Massimiliano Culpo <[email protected]>
@alalazo alalazo force-pushed the maintainers/remove-bootstrapping-compilers branch from f3ea5cc to a58677b Compare September 6, 2024 06:00
@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Sep 6, 2024

@becker33 I rebased the PR. At this point the PR mostly removes entire functions / methods, so it should be easy to check what needs to done in case of conflicts. If we leave removing BuildTask.compiler etc. for #39136 I don't think there will be many conflicts among the two.

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 think the _add_compiler_package_to_config method is unique to this feature, and we can remove it and all of its call-sites. The call sites are all conditions if task.compiler.

The _modify_existing_task is never called once this PR is merged, but the method itself hasn't been removed.

Signed-off-by: Massimiliano Culpo <[email protected]>
Signed-off-by: Massimiliano Culpo <[email protected]>
Signed-off-by: Massimiliano Culpo <[email protected]>
@spackbot-app spackbot-app bot added commands core PR affects Spack core functionality defaults documentation Improvements or additions to documentation stand-alone-tests Stand-alone (or smoke) tests for installed packages tests General test capability(ies) labels Sep 9, 2024
@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Sep 9, 2024

@becker33 Review should be addressed

@alalazo alalazo merged commit ffdfa49 into spack:develop Sep 10, 2024
@alalazo alalazo deleted the maintainers/remove-bootstrapping-compilers branch September 10, 2024 18:02
becker33 added a commit that referenced this pull request Oct 10, 2024
This PR allows users to configure explicit splicing replacement of an abstract spec in the concretizer.

concretizer:
  splice:
    explicit:
    - target: mpi
      replacement: mpich/abcdef
      transitive: true

This config block would mean "for any spec that concretizes to use mpi, splice in mpich/abcdef in place of the mpi it would naturally concretize to use. See #20262, #26873, #27919, and #46382 for PRs enabling splicing in the Spec object. This PR will be the first place the splice method is used in a user-facing manner. See https://spack.readthedocs.io/en/latest/spack.html#spack.spec.Spec.splice for more information on splicing.

This will allow users to reuse generic public binaries while splicing in the performant local mpi implementation on their system.

In the config file, the target may be any abstract spec. The replacement must be a spec that includes an abstract hash `/abcdef`. The transitive key is optional, defaulting to true if left out.

Two important items to note:

1. When writing explicit splice config, the user is in charge of ensuring that the replacement specs they use are binary compatible with whatever targets they replace. In practice, this will likely require either specific knowledge of what packages will be installed by the user's workflow, or somewhat more specific abstract "target" specs for splicing, to ensure binary compatibility.
2. Explicit splices can cause the output of the concretizer not to satisfy the input. For example, using the config above and consider a package in a binary cache `hdf5/xyzabc` that depends on mvapich2. Then the command `spack install hdf5/xyzabc` will instead install the result of splicing `mpich/abcdef` into `hdf5/xyzabc` in place of whatever mvapich2 spec it previously depended on. When this occurs, a warning message is printed `Warning: explicit splice configuration has caused the the concretized spec {concrete_spec} not to satisfy the input spec {input_spec}".

Highlighted technical details of implementation:

1. This PR required modifying the installer to have two separate types of Tasks, `RewireTask` and `BuildTask`. Spliced specs are queued as `RewireTask` and standard specs are queued as `BuildTask`. Each spliced spec retains a pointer to its build_spec for provenance. If a RewireTask is dequeued and the associated `build_spec` is neither available in the install_tree nor from a binary cache, the RewireTask is requeued with a new dependency on a BuildTask for the build_spec, and BuildTasks are queued for the build spec and its dependencies.
2. Relocation is modified so that a spack binary can be simultaneously installed and rewired. This ensures that installing the build_spec is not necessary when splicing from a binary cache.
3. The splicing model is modified to more accurately represent build dependencies -- that is, spliced specs do not have build dependencies, as spliced specs are never built. Their build_specs retain the build dependencies, as they may be built as part of installing the spliced spec.
4. There were vestiges of the compiler bootstrapping logic that were not removed in #46237 because I asked alalazo to leave them in to avoid making the rebase for this PR harder than it needed to be. Those last remains are removed in this PR.

Co-authored-by: Nathan Hanford <[email protected]>
Co-authored-by: Gregory Becker <[email protected]>
Co-authored-by: Tamara Dahlgren <[email protected]>
arezaii pushed a commit to arezaii/spack that referenced this pull request Oct 22, 2024
This PR allows users to configure explicit splicing replacement of an abstract spec in the concretizer.

concretizer:
  splice:
    explicit:
    - target: mpi
      replacement: mpich/abcdef
      transitive: true

This config block would mean "for any spec that concretizes to use mpi, splice in mpich/abcdef in place of the mpi it would naturally concretize to use. See spack#20262, spack#26873, spack#27919, and spack#46382 for PRs enabling splicing in the Spec object. This PR will be the first place the splice method is used in a user-facing manner. See https://spack.readthedocs.io/en/latest/spack.html#spack.spec.Spec.splice for more information on splicing.

This will allow users to reuse generic public binaries while splicing in the performant local mpi implementation on their system.

In the config file, the target may be any abstract spec. The replacement must be a spec that includes an abstract hash `/abcdef`. The transitive key is optional, defaulting to true if left out.

Two important items to note:

1. When writing explicit splice config, the user is in charge of ensuring that the replacement specs they use are binary compatible with whatever targets they replace. In practice, this will likely require either specific knowledge of what packages will be installed by the user's workflow, or somewhat more specific abstract "target" specs for splicing, to ensure binary compatibility.
2. Explicit splices can cause the output of the concretizer not to satisfy the input. For example, using the config above and consider a package in a binary cache `hdf5/xyzabc` that depends on mvapich2. Then the command `spack install hdf5/xyzabc` will instead install the result of splicing `mpich/abcdef` into `hdf5/xyzabc` in place of whatever mvapich2 spec it previously depended on. When this occurs, a warning message is printed `Warning: explicit splice configuration has caused the the concretized spec {concrete_spec} not to satisfy the input spec {input_spec}".

Highlighted technical details of implementation:

1. This PR required modifying the installer to have two separate types of Tasks, `RewireTask` and `BuildTask`. Spliced specs are queued as `RewireTask` and standard specs are queued as `BuildTask`. Each spliced spec retains a pointer to its build_spec for provenance. If a RewireTask is dequeued and the associated `build_spec` is neither available in the install_tree nor from a binary cache, the RewireTask is requeued with a new dependency on a BuildTask for the build_spec, and BuildTasks are queued for the build spec and its dependencies.
2. Relocation is modified so that a spack binary can be simultaneously installed and rewired. This ensures that installing the build_spec is not necessary when splicing from a binary cache.
3. The splicing model is modified to more accurately represent build dependencies -- that is, spliced specs do not have build dependencies, as spliced specs are never built. Their build_specs retain the build dependencies, as they may be built as part of installing the spliced spec.
4. There were vestiges of the compiler bootstrapping logic that were not removed in spack#46237 because I asked alalazo to leave them in to avoid making the rebase for this PR harder than it needed to be. Those last remains are removed in this PR.

Co-authored-by: Nathan Hanford <[email protected]>
Co-authored-by: Gregory Becker <[email protected]>
Co-authored-by: Tamara Dahlgren <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands core PR affects Spack core functionality defaults documentation Improvements or additions to documentation stand-alone-tests Stand-alone (or smoke) tests for installed packages tests General test capability(ies)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants