Skip to content
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

BuildEnvironment: accumulate module changes to poke to all relevant modules #32340

Merged
merged 10 commits into from
Oct 4, 2022

Conversation

becker33
Copy link
Member

Fixes #32336

Currently, module changes from setup_dependent_package are applied only to the module of the package class, but not to any parent classes' modules between the package class module and spack.package_base.

In this PR, we create a custom class to accumulate module changes, and apply those changes to each class that requires it. This design allows us to code for a single module, while applying the changes to multiple modules as needed under the hood, without requiring the user to reason about package inheritance.

@tgamblin you'll probably want to look at this.

@spackbot-app spackbot-app bot added build-environment core PR affects Spack core functionality labels Aug 23, 2022
@becker33
Copy link
Member Author

@spackbot fix style

@spackbot-app
Copy link

spackbot-app bot commented Aug 24, 2022

Let me see if I can fix that for you!

@spackbot-app
Copy link

spackbot-app bot commented Aug 24, 2022

I was able to run spack style --fix for you!

spack style --fix
==> Running style checks on spack
  selected: isort, mypy, black, flake8
==> Modified files
  lib/spack/spack/build_environment.py
==> Running isort checks
  isort checks were clean
==> Running mypy checks
Success: no issues found in 558 source files
  mypy checks were clean
==> Running black checks
reformatted lib/spack/spack/build_environment.py
All done! ✨ 🍰 ✨
1 file reformatted.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> spack style checks were clean
Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I've updated the branch with isort fixes.

@spackbot-app
Copy link

spackbot-app bot commented Aug 24, 2022

It looks like you had an issue with style checks! I can help with that if you ask me! Just say:

@spackbot fix style

... and I'll try to fix style and push a commit to your fork with the fix.

Alternatively, you can run:

$ spack style --fix

And then update the pull request here.

@spackbot-app
Copy link

spackbot-app bot commented Aug 25, 2022

It looks like you had an issue with style checks! I can help with that if you ask me! Just say:

@spackbot fix style

... and I'll try to fix style and push a commit to your fork with the fix.

Alternatively, you can run:

$ spack style --fix

And then update the pull request here.

1 similar comment
@spackbot-app
Copy link

spackbot-app bot commented Aug 25, 2022

It looks like you had an issue with style checks! I can help with that if you ask me! Just say:

@spackbot fix style

... and I'll try to fix style and push a commit to your fork with the fix.

Alternatively, you can run:

$ spack style --fix

And then update the pull request here.

@tgamblin tgamblin self-assigned this Sep 30, 2022
@spackbot-app spackbot-app bot added the tests General test capability(ies) label Oct 3, 2022
@becker33
Copy link
Member Author

becker33 commented Oct 3, 2022

@spackbot fix style

@spackbot-app
Copy link

spackbot-app bot commented Oct 3, 2022

Let me see if I can fix that for you!

@spackbot-app
Copy link

spackbot-app bot commented Oct 3, 2022

I was able to run spack style --fix for you!

spack style --fix
==> Running style checks on spack
  selected: isort, mypy, black, flake8
==> Modified files
  lib/spack/spack/build_environment.py
  lib/spack/spack/test/build_environment.py
  var/spack/repos/builtin.mock/packages/cmake-client-inheritor/package.py
==> Running isort checks
Fixing /tmp/tmpxmage56q/spack/var/spack/repos/builtin.mock/packages/cmake-client-inheritor/package.py
  isort checks were clean
==> Running mypy checks
Success: no issues found in 558 source files
  mypy checks were clean
==> Running black checks
reformatted lib/spack/spack/test/build_environment.py
All done! ✨ 🍰 ✨
1 file reformatted, 2 files left unchanged.
  black checks were clean
==> Running flake8 checks
var/spack/repos/builtin.mock/packages/cmake-client-inheritor/package.py:6: [F401] 'os' imported but unused
  flake8 found errors
Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I've updated the branch with isort fixes.

lib/spack/spack/build_environment.py Outdated Show resolved Hide resolved
lib/spack/spack/build_environment.py Outdated Show resolved Hide resolved
@becker33
Copy link
Member Author

becker33 commented Oct 3, 2022

@spackbot fix style

@spackbot-app
Copy link

spackbot-app bot commented Oct 3, 2022

Let me see if I can fix that for you!

@spackbot-app
Copy link

spackbot-app bot commented Oct 3, 2022

I was able to run spack style --fix for you!

spack style --fix
==> Running style checks on spack
  selected: isort, mypy, black, flake8
==> Modified files
  lib/spack/spack/build_environment.py
  lib/spack/spack/test/build_environment.py
  var/spack/repos/builtin.mock/packages/cmake-client-inheritor/package.py
==> Running isort checks
Fixing /tmp/tmpseclb6dk/spack/var/spack/repos/builtin.mock/packages/cmake-client-inheritor/package.py
  isort checks were clean
==> Running mypy checks
Success: no issues found in 558 source files
  mypy checks were clean
==> Running black checks
All done! ✨ 🍰 ✨
3 files left unchanged.
  black checks were clean
==> Running flake8 checks
var/spack/repos/builtin.mock/packages/cmake-client-inheritor/package.py:6: [F401] 'spack.package.*' imported but unused
  flake8 found errors
Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I've updated the branch with isort fixes.

@tgamblin tgamblin merged commit 5f59821 into develop Oct 4, 2022
@tgamblin tgamblin deleted the bugfix/inherited-packages-module-setup branch October 4, 2022 20:06
@haampie
Copy link
Member

haampie commented Nov 22, 2022

This was a breaking change :(

It's now impossible to read properties from the parent module.

How are build deps like gmake / ninja supposed to get the parent module.make_jobs?

Comment on lines +1009 to +1010
changes = spack.util.pattern.Bunch()
dpkg.setup_dependent_package(changes, spec)
Copy link
Member

@alalazo alalazo Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think many packages would be affected, but this potentially breaks previous semantics since we are not passing something that resembles the module. So if a package was accessing module properties, it won't find them in the Bunch(). Also, the PackageBase docstring is now misleading, since it says we are passing package.module as an argument.

EDIT: Sorry for the double posting, I should have refreshed the browser 😆

alalazo added a commit to alalazo/spack that referenced this pull request Nov 22, 2022
…g build env

This fixes an issue introduced in spack#32340, which changed the semantics of the "module"
object passed to the "setup_dependent_package" callback.
alalazo added a commit to alalazo/spack that referenced this pull request Nov 24, 2022
…g build env

This fixes an issue introduced in spack#32340, which changed the semantics of the "module"
object passed to the "setup_dependent_package" callback.
alalazo added a commit that referenced this pull request Nov 28, 2022
…g build env (#34059)

This fixes an issue introduced in #32340, which changed the semantics of the "module"
object passed to the "setup_dependent_package" callback.
haampie pushed a commit that referenced this pull request Dec 22, 2022
…g build env (#34059)

This fixes an issue introduced in #32340, which changed the semantics of the "module"
object passed to the "setup_dependent_package" callback.
haampie pushed a commit that referenced this pull request Dec 23, 2022
…g build env (#34059)

This fixes an issue introduced in #32340, which changed the semantics of the "module"
object passed to the "setup_dependent_package" callback.
haampie pushed a commit that referenced this pull request Jan 18, 2023
…g build env (#34059)

This fixes an issue introduced in #32340, which changed the semantics of the "module"
object passed to the "setup_dependent_package" callback.
haampie pushed a commit that referenced this pull request Jan 24, 2023
…g build env (#34059)

This fixes an issue introduced in #32340, which changed the semantics of the "module"
object passed to the "setup_dependent_package" callback.
haampie pushed a commit that referenced this pull request Feb 7, 2023
…g build env (#34059)

This fixes an issue introduced in #32340, which changed the semantics of the "module"
object passed to the "setup_dependent_package" callback.
alalazo added a commit that referenced this pull request Feb 7, 2023
…g build env (#34059)

This fixes an issue introduced in #32340, which changed the semantics of the "module"
object passed to the "setup_dependent_package" callback.
amd-toolchain-support pushed a commit to amd-toolchain-support/spack that referenced this pull request Feb 16, 2023
…g build env (spack#34059)

This fixes an issue introduced in spack#32340, which changed the semantics of the "module"
object passed to the "setup_dependent_package" callback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-environment core PR affects Spack core functionality tests General test capability(ies)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inherited packages break module-scoped variables
4 participants