Skip to content

Document test dependency type#18365

Merged
becker33 merged 1 commit intospack:developfrom
adamjstewart:docs/test-deptype
Sep 2, 2020
Merged

Document test dependency type#18365
becker33 merged 1 commit intospack:developfrom
adamjstewart:docs/test-deptype

Conversation

@adamjstewart
Copy link
Copy Markdown
Member

Spack has had a "test" dep type for years, but this hasn't been documented until now. Thanks for bringing this to my attention @KineticTheory.

As for the other dep types, users are constantly confused on their meaning, so let's make this more clear while we're at it.

Closes #3768

@adamjstewart adamjstewart added documentation Improvements or additions to documentation dependencies labels Aug 28, 2020
(building project X doesn't need project Y's build dependencies).
* **"link"**: the project is linked to by the project. The package will be
added to the current package's ``rpath``.
* **"run"**: the project is used by the project at runtime. The package will
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

"the project is used by the project" is possibly the most confusing statement I've ever seen.

this because uninstalling the dependency would break the package.
this because uninstalling the dependency would break the package. Another
consequence of this is that ``build``-only dependencies do not affect the
hash of the package. The same is true for ``test`` dependencies.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying this point. I didn't realize this behavior was available.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For build dependencies this might change in the future, so not sure if we should state it in the docs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My understanding was that in #3768 we decided the best idea would be to add an include deptype for things that affect the hash but can be uninstalled after installation.

Anyway, based on how long #3768 has been sitting without any changes, I would rather document the current behavior than document some future behavior.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My understanding was that in #3768 we decided the best idea would be to add an include deptype for things that affect the hash but can be uninstalled after installation.

@adamjstewart The dependency model discussed in #3768 is outdated in the sense that:

  1. A build dependency might affect how a package binary is built, so it should affect the hash since we use that for reproducibility (think of a cmake that changes some compilation defaults across versions)
  2. There are other aspects to be taken into consideration, for instance if a dependency is unified over a DAG or not (one can build 10 different nodes of a DAG with 10 different cmake, but likely can't use different versions of the same C++ template library in a single DAG)

I concur that what is documented here is the current implementation and I'm fine with these changes overall. Can we maybe add a note or warning box to state that we might change how build deps affect the hash in the future if extending some aspect of the dependency model require it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

  1. I don't want to think about this 😦 . I agree it could be problematic, but having the build dep affect the hash will cause a lot of reinstalls. I guess we'll probably wait until the new concretizer for that so there are fewer rebuilds.
  2. I would argue that such a C++ template library should be a type=link or type=include dependency, but I don't know as much about C++ so maybe I'm wrong.

I'll wait for one more reviewer to suggest adding a note about how these might change in the future. I'm trying to be as detailed as possible without confusing the reader. I assume if we change the behavior of any of these deptypes, we will update any packages that need to be updated accordingly. So it's not as important (to me at least) to warn users of future behavior in the Packaging Guide.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess we'll probably wait until the new concretizer for that so there are fewer rebuilds.

That's in fact the first use case I have in mind. Fine with me to wait for other reviewers and see what they think.

I'll wait for one more reviewer to suggest adding a note about how these might change in the future. I'm trying to be as detailed as possible without confusing the reader.

Just to be explicit on my intent and avoid misunderstanding: I'm not proposing to document now how things will change, but just suggesting to add a brief note to users to say that the behavior of build deps might still change in the future.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think documenting the current behavior and changing the documentation as needed is the proper course of action.

Copy link
Copy Markdown
Contributor

@KineticTheory KineticTheory left a comment

Choose a reason for hiding this comment

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

LGTM

@becker33 becker33 merged commit 1992fdf into spack:develop Sep 2, 2020
@adamjstewart adamjstewart deleted the docs/test-deptype branch September 2, 2020 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document the meaning of deptypes

4 participants