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

Perform concretization of build-only deps separately #2548

Closed

Conversation

scheibelp
Copy link
Member

@scheibelp scheibelp commented Dec 10, 2016

This takes each dependency which has only the 'build' deptype and performs a distinct concretization on it. These build-only deps are no longer explicitly part of the dependency DAG. The dependency DAG may now include multiple instances of a given package, namely when it appears as a link dependency of the root and as a dependency of some package that is a build dependency.

A common example is the cmake package: many packages may depend on cmake as a build dependency; cmake depends on zlib transitively. zlib is also a common dependency (transitively). With this PR, packages which depend on zlib via cmake and by linking via another package will build two instances of zlib.

It is not always possible to consider the concretization of a build-only dependency separately from other dependencies. At the moment this PR ignores this possibility and always performs a separate concretization.

TODOs:

See:

@davydden
Copy link
Member

can you please add conretization test with 3 mockup packages:

A -> B [build-only] -> C+variant
  \
    \ [link] -> C~variant

to make sure this feature won't get broken in future.

@@ -41,6 +41,8 @@ def setup_parser(subparser):
default='nodes', choices=['nodes', 'edges', 'paths'],
help='How extensively to traverse the DAG. (default: nodes).')
subparser.add_argument(
'--merge-build', action='store_true', default=False)
Copy link
Member

Choose a reason for hiding this comment

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

perhaps add description of this argument in help=...

@adamjstewart
Copy link
Member

A common example is the cmake package: many packages may depend on cmake as a build dependency; cmake depends on zlib transitively. zlib is also a common dependency (transitively). With this PR, packages which depend on zlib via cmake and by linking via another package will build two instances of zlib.

I'm not sure I understand this example. Are you saying that if package A depends on zlib@1 and package B depends on cmake which depends on zlib@2, it will build two instances? Or are you saying it will always build two instances of zlib regardless of the version constraints? I think a more clear use case that has recently been a headache is trying to build Python 3 packages that have a link dependency on Python 3, but require GUI packages which have a build dependency on Python 2. In that case, the only way to build them is to build 2 different copies of Python, making this PR very useful.

@scheibelp
Copy link
Member Author

I think a more clear use case that has recently been a headache is trying to build Python 3 packages that have a link dependency on Python 3, but require GUI packages which have a build dependency on Python 2.

That will be another intended use case to satisfy with this - thanks for adding it!

Are you saying that if package A depends on zlib@1 and package B depends on cmake which depends on zlib@2, it will build two instances? Or are you saying it will always build two instances of zlib regardless of the version constraints?

If the version of zlib needed by cmake and by package A were the same, it would not install the same zlib twice. However when concretizing zlib as part of cmake it will not account for any constraints on zlib placed by package A, which is to say if there was some zlib@1.5 that satisfied both cmake and package A, it would not be found in this PR.

@scheibelp
Copy link
Member Author

can you please add concretization test with 3 mockup packages

Added TODO - thanks @davydden

@citibeth
Copy link
Member

It is not always possible to consider the concretization of a build-only dependency separately from other dependencies.

Can you expand on this please?

@scheibelp
Copy link
Member Author

It is not always possible to consider the concretization of a build-only dependency separately from other dependencies.

Can you expand on this please?

This occurs when a child of a build dependency conflicts with another dependency; what precisely constitutes a conflict is TBD - right now I'm just working with examples. For example:

X-[b]->Y-[r]->python2
X-[L, b]->python3

(note this particular case is not known to exist)

Here X is a python3 module which uses package Y to build. Y runs python2. In this case there is competition for PYTHONPATH.

@citibeth
Copy link
Member

citibeth commented Dec 13, 2016 via email

@becker33
Copy link
Member

I think @scheibelp is worried about the case in which Y depends on python2 not as a build dep.

@scheibelp
Copy link
Member Author

Wouldn't you need to concretize each build dependency separately?

In this case even if you do, I think there is a question of how to properly set up PYTHONPATH when the time comes to build X - the separate concretizations would work fine but then there is an issue of properly referencing these dependencies from X.

X needs python3 and Y needs python2 to run, so for example I think it would be difficult to set up PYTHONPATH properly. Also setting up the reference to the 'python' executable (from the perspective of X) would have a conflict.

Perhaps most importantly I should say I don't think this will be a common issue; like I said for the example I put forth I don't know of any actual cases of it. I was going to put more effort in detecting these issues later on in the PR.

Another potential generic example could be:

X-[b]->Y-[L]->Z1
X-[L]->Z2

I say "could be" because I think RPATHs would generally avoid a conflict between Z1 and Z2, however if a system requires modifying LD_LIBRARY_PATH to reference libs (i.e. RPATH is not used) there would be a conflict. (NOTE: I'm not actually sure if that applies anywhere)

@citibeth
Copy link
Member

In this case even if you do, I think there is a question of how to properly set up PYTHONPATH when the time comes to build X

I see, thinks. I suppose that how you set $PYTHONPATH is a fundamental problem, whether or not Spack is involved. Such a package would be unbuildable, without some special Python Jujitsu in the package itself.

…ies transitively for setup_dependent package and PKG_CONFIG_PATH (NOTE) still need to merge intersecting build-only deps
@scheibelp
Copy link
Member Author

I see, thinks. I suppose that how you set $PYTHONPATH is a fundamental problem, whether or not Spack is involved. Such a package would be unbuildable, without some special Python Jujitsu in the package itself.

Agreed - and to be clear I'm not focused on making such cases work for this PR, but perhaps I at least want to detect them.

@scheibelp
Copy link
Member Author

This work is motivating a new dependency type: 'include'

This comes up for the libxcb dependency in python+tk. The following explicit dependencies exist

libxcb -[b, L]-> libxau -[b]-> xproto

There is also an implicit dependency (not in a package.py)

libxcb -[b]-> xproto

Build-only deps are presumed to essentially be tools that are run at build time and so can be built separately, for example building two separate instances of cmake is OK. xproto is a header library which should match between libxcb and libxau; even though it is only needed during build time it should not concretize separately. Therefore it should be an 'include' dependency.

In all the 4 deptypes would be:

  • build: Add only to PATH, needs to be built for front-end. concretizes separately (unlike the other 3)
  • include: Only needed for include at build time, not for linking
  • link: Implies include, also sets RPATHs
  • run: needed for running (e.g. also need to set PATH when used by build deps)

… to path. for now force contained build deps to concretize together but add all of them explicitly. perhaps adding run/link deps of build deps to path is the better option
… by forcing a separate concretization from that top-level package which only considers the build-only dependencies
@svenevs
Copy link
Member

svenevs commented May 9, 2017

Hey, there might be a problem not worth actually solving, but worth explaining. Currently, mixed python is going to give you an error:

Concretized
--------------------------------
==> Error: Separate concretization of python produced build-time conflict

This is generally probably what you want. However, suppose I want to build opencv+vtk or +qt. Both vtk and qt (in my experience), cannot be built with python3. More specifically, vtk has some hardcoded py27 library trying to be built.

This now means that I am unable to build opencv+python ^python@3.6.1. Opencv is kind of a special snowflake, so I'm not sure how many other packages will be affected by this. The only workaround I can think of is currently you're gathering all of my build dependencies. If something is already built, this is not necessary. E.g. since opencv+qt depends on qt, if I the user give you a specific hash for qt that is already built, don't inherit the qt build dependencies as well?

@scheibelp
Copy link
Member Author

I am unable to build opencv+python ^python@3.6.1

@svenevs

It turns out this is another instance of #2548 (comment) that occurs specifically with py-numpy. Unfortunately since my error message is terrible it is hard for anyone to tell.

If I edit the py-setuptools dependency in py-numpy to be depends_on('py-setuptools', type=('build', 'link')) then spack spec -t opencv+python ^python@3.6.1 succeeds.

At the moment I'm prioritizing fixing this issue over improving the error message (although both are essential) and moreover progress on this PR altogether has been slow on this because I've been prioritizing other work.

@svenevs
Copy link
Member

svenevs commented May 10, 2017

At the moment I'm prioritizing fixing this issue over improving the error message (although both are essential) and moreover progress on this PR altogether has been slow on this because I've been prioritizing other work.

Claro. I will stop including additional information, I thought this was different. I'm definitely not trying to rush anything...just trying to give user-testing!

@scheibelp
Copy link
Member Author

Claro. I will stop including additional information, I thought this was different. I'm definitely not trying to rush anything...just trying to give user-testing!

Whoops sorry I didn't mean to imply I don't want to know about issues, just that development is going slow on this. I really appreciate the testing folks have done on this! I definitely don't mind checking into problems people have.

I should also mention: part of the reason this has been prioritized lower is that @tgamblin may be able to incorporate support for this into his more comprehensive update to the concretization logic. That being said, issues that people want to report here will likely be useful as use cases for his work.

@svenevs
Copy link
Member

svenevs commented May 10, 2017

Oh yeah no I meant to say with respect to the python stuff because it sounds like they all generally stem from the same thing! I don't want to keep flooding this issue with things that are all the same cause, so if I have python specific issues I'm just gonna hack around them.

diaena pushed a commit to diaena/spack that referenced this pull request May 26, 2017
…pack#4158)

* adding paramiko and missing dependencies, setup to work with spack#2548

* adding other deps for paramiko

* fix flake8 errors

* removed spurious add

* address suggestion for proper dependencies

* fix cryptography deps

* remove FIXME comments and commented depends lines
…conflict (2) apply py-setuptools hack to get py-jupyter-notebook building
@syntion
Copy link

syntion commented Jun 2, 2017

While working on #4319 I noticed, that spack fetch -D doesn't fetch the packages required for building.

xavierandrade pushed a commit to xavierandrade/spack that referenced this pull request Jun 16, 2017
…pack#4158)

* adding paramiko and missing dependencies, setup to work with spack#2548

* adding other deps for paramiko

* fix flake8 errors

* removed spurious add

* address suggestion for proper dependencies

* fix cryptography deps

* remove FIXME comments and commented depends lines
EmreAtes pushed a commit to EmreAtes/spack that referenced this pull request Jul 10, 2017
…pack#4158)

* adding paramiko and missing dependencies, setup to work with spack#2548

* adding other deps for paramiko

* fix flake8 errors

* removed spurious add

* address suggestion for proper dependencies

* fix cryptography deps

* remove FIXME comments and commented depends lines
@tgamblin tgamblin modified the milestones: v1.0, v0.12.0 Nov 12, 2017
@healther
Copy link
Contributor

healther commented Dec 6, 2017

I just opened #6589 because of the unrestricted py-setuptools dependency in py-flake8, but add this means that we can no longer build our meta package (including py-ipython and py-flake8).
Work around right now will probably be: exclude py-ipython from metapackage and manually add to the produced module/filesystemview. It would be nice if there would be an easier way.

@citibeth
Copy link
Member

citibeth commented May 1, 2018

Once this PR is merged, all variants named python3 should be removed; basically, #7926 should be revereted.

@becker33
Copy link
Member

Closing; superseded by #4595

@becker33 becker33 closed this Apr 24, 2019
luigi-calori pushed a commit to RemoteConnectionManager/spack_deploy_helper that referenced this pull request Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet