Separate setting build environment and run environment in packages by alalazo · Pull Request #11115 · spack/spack

alalazo · 2019-04-05T12:35:05Z

fixes #6143
fixes #2016

refers to #2016 (comment)

As a package maintainer I want different methods to specify the build-time environment and the run-time environment of a package, so that I will know exactly which are the preconditions of the method that is being called.

Rationale

Currently each package can override two methods:

def setup_environment(self, build_env, run_env):
    ...

def setup_dependent_environment(self, build_env, run_env, dependent_spec):
    ...

to modify its build or run environment, or those of a dependent. This has a fundamental issue in that the same method will be called at different times, and will have different preconditions depending when it's called:

If it is called to set-up the build environment the prefix of the spec being installed is not yet there + a few special variables that are needed to build with Spack compiler wrappers will be set
If it is called to set-up the run environment the underlying spec is assumed to be already installed

This in turn might cause issues like #6143 or #2016 (comment). In this PR we split each of the two methods above, e.g. :

def setup_build_environment(self, env):
   ...

def setup_run_environment(self, env):
   ...

so that it will be clear which are the preconditions of each method call.

Description

This PR splits each *_environment method in spack.package.PackageBase into two: one that will be called for the build-time environment and one that wil be called for the run-time. As there are a lot of packages overriding these callbacks the default implementation is such to mimic the legacy behavior, and emit a deprecation warning to tty.debug, e.g.:

$ spack -d install mpich
[...]
==> [2019-04-05-14:13:38.193971] Installing mpich
==> [2019-04-05-14:13:38.194114] Searching for binary cache of mpich
==> [2019-04-05-14:13:38.194407] Reading config file /home/mculpo/.spack/linux/mirrors.yaml
==> [2019-04-05-14:13:38.195858] Finding buildcaches in /home/mculpo/production/mirror/build_cache
==> [2019-04-05-14:13:38.198158] No binary for mpich found: installing from source
==> [2019-04-05-14:13:38.300407] [DEPRECATED METHOD]
"pkgconf" still defines the deprecated method "setup_dependent_environment" [should be split into "setup_dependent_build_environment" and "setup_dependent_run_environment"]
==> [2019-04-05-14:13:38.323784] [DEPRECATED METHOD]
"perl" still defines the deprecated method "setup_dependent_environment" [should be split into "setup_dependent_build_environment" and "setup_dependent_run_environment"]
==> [2019-04-05-14:13:38.344990] [DEPRECATED METHOD]
"libtool" still defines the deprecated method "setup_dependent_environment" [should be split into "setup_dependent_build_environment" and "setup_dependent_run_environment"]
[...]

Additional information

Split setup_environment into two methods
Split setup_dependent_environment into two methods
Add documentation in the "Packaging guide" section of the docs
Update the tutorial sections of the docs
Add a test that fails if a package under PR still implements the old methods (this can be used to make the community aware of the new methods, can be removed once the old methods are not supported anymore)
Add a test that fails if any package implements the old methods, and mark it xfail (it will be useful when all packages are ported, in case of people doing PR based on old behavior)

alalazo · 2019-04-05T12:35:42Z

I added this PR as a draft to collect opinions on splitting the two methods before proceeding with docs and tests. If you don't have major concerns on what's being done here we can use the PR also to improve the APIs in build_environment.py and serve cases like #11084. Let me know what you think.

becker33

I didn't do a particularly deep review, just the one comment for now.

I think this is probably a good direction to be moving in.

lib/spack/spack/build_environment.py

tgamblin · 2019-04-16T18:12:08Z

No deep review yet, but this is a much cleaner, less confusing direction than the current state of affairs. I'm in favor!

alalazo · 2019-09-19T15:58:13Z

Ready for a review from my side. The description at the top should be up to date. Unit tests have been added and docs updated.

becker33

@alalazo only one comment at the moment. I'm a little concerned about build/link dependency semantics.

lib/spack/spack/build_environment.py

Before this commit the `*_environment` methods were setting modifications to both the build-time and run-time environment simultaneously. This might cause issues as the two environments inherently rely on different preconditions: 1. The build-time environment is set before building a package, thus the package prefix doesn't exist and can't be inspected 2. The run-time environment instead is set assuming the target package has been already installed Here we split each of these functions into two: one setting the build-time environment, one the run-time. We also adopt a fallback strategy that inspects for old methods and executes them as before, but prints a deprecation warning to tty. This permits to port packages to use the new methods in a distributed way, rather than having to modify all the packages at once.

Also, emit a debug warning to avoid polluting messages on tty.

fixes spack#6145

Marked the test xfail for now as we have a lot of packages in that state.

This test can be used any time we deprecate a method call to ensure that during the first modification of the package we update also the deprecated calls.

…pack#11115) * Methods setting the environment now do it separately for build and run Before this commit the `*_environment` methods were setting modifications to both the build-time and run-time environment simultaneously. This might cause issues as the two environments inherently rely on different preconditions: 1. The build-time environment is set before building a package, thus the package prefix doesn't exist and can't be inspected 2. The run-time environment instead is set assuming the target package has been already installed Here we split each of these functions into two: one setting the build-time environment, one the run-time. We also adopt a fallback strategy that inspects for old methods and executes them as before, but prints a deprecation warning to tty. This permits to port packages to use the new methods in a distributed way, rather than having to modify all the packages at once. * Added a test that fails if any package uses the old API Marked the test xfail for now as we have a lot of packages in that state. * Added a test to check that a package modified by a PR is up to date This test can be used any time we deprecate a method call to ensure that during the first modification of the package we update also the deprecated calls. * Updated documentation

- build: no special treatment - already addressed in the spack-Allwmake script - run: use environment from the OpenFOAM etc/bashrc

…3645) - build: no special treatment - already addressed in the spack-Allwmake script - run: use environment from the OpenFOAM etc/bashrc

See spack#11115

* add package [email protected] add CUDA 10.2 support * CudaPackage: Add 10.2 Conflicts * CUDA: Modernize Run Environment See #11115

split setup_environment (see spack/spack#11115)

fixes spack#29446 The new setup_*_environment functions have been falling back to calling the old functions and warn the user since spack#11115. This commit removes the fallback behavior and any use of: - setup_environment - setup_dependent_environment in the codebase

fixes #29446 The new setup_*_environment functions have been falling back to calling the old functions and warn the user since #11115. This commit removes the fallback behavior and any use of: - setup_environment - setup_dependent_environment in the codebase

alalazo added feature A feature is missing in Spack build-error modules build-environment environments labels Apr 5, 2019

alalazo requested review from adamjstewart, becker33 and scheibelp April 5, 2019 12:35

becker33 requested changes Apr 9, 2019

View reviewed changes

lib/spack/spack/build_environment.py Outdated Show resolved Hide resolved

alalazo self-assigned this Sep 19, 2019

alalazo force-pushed the features/divide_build_from_run_env branch from 6060f2b to 74034f3 Compare September 19, 2019 10:40

alalazo marked this pull request as ready for review September 19, 2019 10:40

alalazo requested a review from tgamblin September 19, 2019 15:57

sethrj mentioned this pull request Sep 22, 2019

Fix GCC environment variables for external installations #12454

Merged

scheibelp mentioned this pull request Oct 1, 2019

Gcc package: setup environment bug when installing #13000

Merged

This was referenced Oct 11, 2019

specify preinstalled numpy in ~/.spack/packages.yaml #13190

Closed

Fix external numpy package support #13197

Merged

becker33 requested changes Oct 15, 2019

View reviewed changes

lib/spack/spack/build_environment.py Outdated Show resolved Hide resolved

alalazo added 9 commits October 16, 2019 10:07

Factored retrieving legacy methods into its own function

305d646

Also, emit a debug warning to avoid polluting messages on tty.

Ported mpich to the new methods

a1fa5e9

Removed code duplication when constructing environment modifications

ba22cb5

Ported elpa to new workflow to set environment variables

79bf32c

fixes spack#6145

Added a test that fails if any package uses the old API

88ab703

Marked the test xfail for now as we have a lot of packages in that state.

Added a test to check that a package modified by a PR is up to date

d283630

This test can be used any time we deprecate a method call to ensure that during the first modification of the package we update also the deprecated calls.

Renamed function to better convey its purpose

0750c16

Updated packaging guide and packages used to showcase overriding

a5fd6b4

becker33 approved these changes Oct 17, 2019

View reviewed changes

becker33 merged commit 9ddc98e into spack:develop Oct 17, 2019

alalazo deleted the features/divide_build_from_run_env branch October 17, 2019 17:18

scheibelp mentioned this pull request Oct 17, 2019

env activation: use package defined env setup methods #13249

Merged

paulbry mentioned this pull request Oct 22, 2019

Added go 1.13.2 and 1.12.11 #13254

Merged

healther mentioned this pull request Oct 25, 2019

LLVM code-signing on OSX: link to the offical documentation #13428

Merged

This was referenced Oct 25, 2019

metabat: add versions 2.14 and 2.13 #13369

Merged

Add Open MPI v. 4.0.2 #13257

Merged

Add Paraview 5.7.0 #13255

Merged

This was referenced Oct 28, 2019

Fix gcc recipe for RHEL7. #13460

Merged

libxc: download from gitlab #13471

Merged

adamjstewart mentioned this pull request Oct 31, 2019

Flang Oct 2019 Update #13510

Closed

alalazo mentioned this pull request Nov 7, 2019

Don't read Python env unless it is installed #13589

Merged

ax3l added a commit to psychocoderHPC/spack that referenced this pull request Nov 25, 2019

CUDA: Modernize Run Environment

cc3f409

See spack#11115

ax3l mentioned this pull request Nov 25, 2019

add package [email protected] #13819

Merged

adamjstewart pushed a commit that referenced this pull request Nov 25, 2019

add package [email protected] (#13819)

9c10b80

* add package [email protected] add CUDA 10.2 support * CudaPackage: Add 10.2 Conflicts * CUDA: Modernize Run Environment See #11115

jasonfenglu added a commit to jasonfenglu/spack that referenced this pull request Mar 24, 2020

[pgplot] update to new api, see spack#11115

443a8e6

jasonfenglu added a commit to jasonfenglu/spack that referenced this pull request Mar 26, 2020

[pgplot] update to new api, see spack#11115

6af26ce

tldahlgren mentioned this pull request May 11, 2021

Advanced packaging needs updating wrt setup_environment spack/spack-tutorial#125

Closed

fangohr added a commit to fangohr/oommf-in-spack that referenced this pull request Sep 30, 2021

Feedback from running spack tests:

9d3e775

split setup_environment (see spack/spack#11115)

fangohr mentioned this pull request Sep 30, 2021

Feedback from running spack tests: split up setup_environment fangohr/oommf-in-spack#25

Merged

fangohr added a commit to fangohr/oommf-in-spack that referenced this pull request Sep 30, 2021

Feedback from running spack tests:

7d01ee6

split setup_environment (see spack/spack#11115)

adamjstewart mentioned this pull request Mar 11, 2022

AutotoolsPackage no longer supports legacy setup_environment #29446

Closed

3 tasks

alalazo mentioned this pull request Mar 11, 2022

Remove "setup_environment" and "setup_dependent_environment" #29463

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate setting build environment and run environment in packages#11115

Separate setting build environment and run environment in packages#11115
becker33 merged 11 commits intospack:developfrom
epfl-scitas:features/divide_build_from_run_env

alalazo commented Apr 5, 2019 •

edited

Loading

Uh oh!

alalazo commented Apr 5, 2019

Uh oh!

becker33 left a comment

Uh oh!

Uh oh!

tgamblin commented Apr 16, 2019 •

edited

Loading

Uh oh!

alalazo commented Sep 19, 2019

Uh oh!

becker33 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

alalazo commented Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Description

Additional information

Uh oh!

alalazo commented Apr 5, 2019

Uh oh!

becker33 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tgamblin commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Sep 19, 2019

Uh oh!

becker33 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alalazo commented Apr 5, 2019 •

edited

Loading

tgamblin commented Apr 16, 2019 •

edited

Loading