A package's build environment should include run dep paths as well as build dep paths#3347
A package's build environment should include run dep paths as well as build dep paths#3347krafczyk wants to merge 5 commits intospack:developfrom
Conversation
|
@certik Could you pull this branch and confirm if it solves your problem? |
|
I applied the following patch: +certik@redhawk:~/repos/spack(develop)$ git diff
diff --git a/var/spack/repos/builtin/packages/npm/package.py b/var/spack/repos/builtin/packages/npm/package.py
index 500674d..6196da4 100644
--- a/var/spack/repos/builtin/packages/npm/package.py
+++ b/var/spack/repos/builtin/packages/npm/package.py
@@ -36,7 +36,7 @@ class Npm(AutotoolsPackage):
version('3.10.9', 'ec1eb22b466ce87cdd0b90182acce07f')
version('3.10.5', '46002413f4a71de9b0da5b506bf1d992')
- depends_on('node-js')
+ depends_on('node-js', type=('build', 'run'))
def setup_dependent_environment(self, spack_env, run_env, dependent_spec):
npm_config_cache_dir = "%s/npm-cache" % dependent_spec.prefixbut still the same problem: |
|
@certik let me take a closer look this weekend. Ill get back to you.
…On Mar 3, 2017 1:40 PM, "Ondřej Čertík" ***@***.***> wrote:
I applied the following patch:
***@***.***:~/repos/spack(develop)$ git diffdiff --git a/var/spack/repos/builtin/packages/npm/package.py b/var/spack/repos/builtin/packages/npm/package.py
index 500674d..6196da4 100644--- a/var/spack/repos/builtin/packages/npm/package.py+++ b/var/spack/repos/builtin/packages/npm/package.py@@ -36,7 +36,7 @@ class Npm(AutotoolsPackage):
version('3.10.9', 'ec1eb22b466ce87cdd0b90182acce07f')
version('3.10.5', '46002413f4a71de9b0da5b506bf1d992')
- depends_on('node-js')+ depends_on('node-js', type=('build', 'run'))
def setup_dependent_environment(self, spack_env, run_env, dependent_spec):
npm_config_cache_dir = "%s/npm-cache" % dependent_spec.prefix
but still the same problem:
***@***.***:~$ spack install -j16 py-jupyter-notebook ***@***.***
==> Installing py-jupyter-notebook
==> Installing npm
==> node-js is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/node-js-7.1.0-qiwhfgccrcfmkgc4m7jx3nkqzmkicw2s
==> Using cached archive: /home/certik/repos/spack/var/spack/cache/npm/npm-3.10.9.tgz
==> Staging archive: /home/certik/repos/spack/var/spack/stage/npm-3.10.9-iukblzfg6e5szeiv2hx6frftl7in4n4q/npm-3.10.9.tgz
==> Created stage in /home/certik/repos/spack/var/spack/stage/npm-3.10.9-iukblzfg6e5szeiv2hx6frftl7in4n4q
==> Ran patch() for npm
==> Building npm [AutotoolsPackage]
==> Executing phase : 'autoreconf'
==> Executing phase : 'configure'
==> Executing phase : 'build'
==> Executing phase : 'install'
==> Successfully installed npm
Fetch: 0.03s. Build: 16.82s. Total: 16.85s.
[+] /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/npm-3.10.9-iukblzfg6e5szeiv2hx6frftl7in4n4q
==> py-ipykernel is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/py-ipykernel-4.5.0-t7irkijqivrpi5sftt5tpuurxwyhojqs
==> py-nbconvert is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/py-nbconvert-4.2.0-i2gzxungrqq63y5vh4znnpdl7l2buwgo
==> py-traitlets is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/py-traitlets-4.3.1-ndrwnw254vls42fyziwyzje5bt6egex6
==> py-ipython-genutils is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/py-ipython-genutils-0.1.0-erlina4ulsiyua6kvek6jxrg47jjbhon
==> py-jupyter-client is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/py-jupyter-client-4.4.0-iacniz2tlakoo7p6u2i2x7krqzyzaxnm
==> py-nbformat is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/py-nbformat-4.1.0-3yi463eaoljgpszmye4ckrddzpwwvk4b
==> python is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/python-2.7.13-5gx5sb5s5ns2cmne3qwnl632omokfdqu
==> py-tornado is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/py-tornado-4.4.0-6hrsmmcsggsmp25ozqy3gtuvh5fhfk5u
==> py-jupyter-core is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/py-jupyter-core-4.2.0-2jjfbcjpfsoqc3ov2lmiucgogeoeavuv
==> py-setuptools is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/py-setuptools-34.2.0-cudzrdzpou7mif2zw3fmmuwhdngfm2x6
==> py-jinja2 is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/py-jinja2-2.8-4ddiwza25irn35vhb6kck6zfyur4xbow
==> py-jupyter-console is already installed in /home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/py-jupyter-console-5.0.0-dm2s57qafex64yne4tbqm4rbsqsza2im
==> Using cached archive: /home/certik/repos/spack/var/spack/cache/py-jupyter-notebook/py-jupyter-notebook-4.2.3.tar.gz
==> Staging archive: /home/certik/repos/spack/var/spack/stage/py-jupyter-notebook-4.2.3-rkbpngcvfiepja6w64zxwovymjw5ms4j/4.2.3.tar.gz
==> Created stage in /home/certik/repos/spack/var/spack/stage/py-jupyter-notebook-4.2.3-rkbpngcvfiepja6w64zxwovymjw5ms4j
==> No patches needed for py-jupyter-notebook
==> Building py-jupyter-notebook [PythonPackage]
==> Executing phase : 'build'
==> Error: ProcessError: Command exited with status 1:
'/home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/python-2.7.13-5gx5sb5s5ns2cmne3qwnl632omokfdqu/bin/python' 'setup.py' '--no-user-cfg' 'build'
/home/certik/repos/spack/lib/spack/spack/build_systems/python.py:110, in python:
109 def python(self, *args):
>> 110 inspect.getmodule(self).python(*args)
See build log for details:
/home/certik/tmp/spack-stage/spack-stage-UDMQdl/notebook-4.2.3/spack-build.out
***@***.***:~$ cat /home/certik/tmp/spack-stage/spack-stage-UDMQdl/notebook-4.2.3/spack-build.out
==> '/home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/python-2.7.13-5gx5sb5s5ns2cmne3qwnl632omokfdqu/bin/python' 'setup.py' '--no-user-cfg' 'build'
running build
running build_py
running jsversion
running css
running jsdeps
installing build dependencies with npm
> npm install
/usr/bin/env: ‘node’: No such file or directory
rebuilding js and css failed. The following required files are missing: ['notebook/static/components', 'notebook/static/notebook/js/main.min.js', 'notebook/static/tree/js/main.min.js', 'notebook/static/edit/js/main.min.js', 'notebook/static/terminal/js/main.min.js', 'notebook/static/auth/js/main.min.js', 'notebook/static/style/ipython.min.css', 'notebook/static/style/style.min.css']
Traceback (most recent call last):
File "setup.py", line 198, in <module>
main()
File "setup.py", line 195, in main
setup(**setup_args)
File "/home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/python-2.7.13-5gx5sb5s5ns2cmne3qwnl632omokfdqu/lib/python2.7/distutils/core.py", line 151, in setup
dist.run_commands()
File "/home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/python-2.7.13-5gx5sb5s5ns2cmne3qwnl632omokfdqu/lib/python2.7/distutils/dist.py", line 953, in run_commands
self.run_command(cmd)
File "/home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/python-2.7.13-5gx5sb5s5ns2cmne3qwnl632omokfdqu/lib/python2.7/distutils/dist.py", line 972, in run_command
cmd_obj.run()
File "/home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/python-2.7.13-5gx5sb5s5ns2cmne3qwnl632omokfdqu/lib/python2.7/distutils/command/build.py", line 127, in run
self.run_command(cmd_name)
File "/home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/python-2.7.13-5gx5sb5s5ns2cmne3qwnl632omokfdqu/lib/python2.7/distutils/cmd.py", line 326, in run_command
self.distribution.run_command(command)
File "/home/certik/repos/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/python-2.7.13-5gx5sb5s5ns2cmne3qwnl632omokfdqu/lib/python2.7/distutils/dist.py", line 972, in run_command
cmd_obj.run()
File "/home/certik/tmp/spack-stage/spack-stage-UDMQdl/notebook-4.2.3/setupbase.py", line 549, in run
raise e
subprocess.CalledProcessError: Command '['npm', 'install']' returned non-zero exit status 127
+
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3347 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGJpnLLBI38vWToO8v0Qm3I-HslAMObeks5riGjGgaJpZM4MSoIQ>
.
|
|
Thanks @krafczyk. |
|
@certik try this branch again, you'll need to force pull fix/npm-bug because I've rebased it on develop. Thanks for reporting this, it turns out this was a more subtle bug than I had anticipated. There was a fairly serious bug hiding here, and I believe we've fixed it. @davydden Let me know your opinion here, It turns out that the bin dirs of only build deps were being added to the environment. To fix this bug, I had to add run dependency bin dirs as well as bin dirs of the build dependencies. @citibeth I vaguely remember discussing where |
lib/spack/spack/build_environment.py
Outdated
| env.set('SPACK_COMPILER_EXTRA_RPATHS', extra_rpaths) | ||
|
|
||
| # Add bin directories from dependencies to the PATH for the build. | ||
| bin_dirs = reversed(filter(os.path.isdir, [ |
There was a problem hiding this comment.
i am not a python guru, but it looks to me that the original code should have added bin dir of node-js.
There was a problem hiding this comment.
It does for npm, but not for packages that depend on npm. The bin dirs are only added for explicit build dependencies. I don't think that package maintainers should have to specify that a package depending on npm also depends on node-js. That ought to come with using npm as a build dependency.
| for item in bin_dirs: | ||
| env.prepend_path('PATH', item) | ||
| # Add bin directories from dependencies to the PATH for the build or run. | ||
| def add_dir_to_list(bin_dir, list): |
There was a problem hiding this comment.
i think we generally do not define functions within other functions.
There was a problem hiding this comment.
Why? It makes the following code much easier to comprehend.
There was a problem hiding this comment.
you misunderstood me, i meant to move the definition outside.
lib/spack/spack/build_environment.py
Outdated
| run_bin_dirs = [] | ||
| for d in pkg.spec.traverse(root=False, deptype='run'): | ||
| add_dir_to_list("%s/bin" % d.prefix, run_bin_dirs) | ||
| run_bin_dirs = filter(os.path.isdir, ['%s/bin' % d.prefix for d in pkg.spec.traverse(root=False, deptype='run') ]) |
There was a problem hiding this comment.
i think you set this list already above. This line and the next can be removed.
lib/spack/spack/build_environment.py
Outdated
| if bin_dir not in list: | ||
| list.append(bin_dir) | ||
| build_deps = [ d for d in pkg.spec.dependencies(deptype='build') ] | ||
| build_bin_dirs = [] |
There was a problem hiding this comment.
could you add a short comment above build_bin_dirs = [], run_bin_dirs = [] and bin_dirs = [] what those are meant to contain.
There was a problem hiding this comment.
(one can of course get it from the code, but I think it makes the function easier to read. It also makes it clear what is the intention here)
|
@davydden I've pushed a new commit to clean up the code I've added a bit. I tried to address your concerns, Please have another look. |
davydden
left a comment
davydden
left a comment
There was a problem hiding this comment.
looks good, but I am sure others would like to have a look.
lib/spack/spack/build_environment.py
Outdated
|
|
||
| # Add all bin directories which build dependencies and their | ||
| # run dependencies require. | ||
| for bdep in [ d for d in pkg.spec.dependencies(deptype='build') ]: |
There was a problem hiding this comment.
Isn't this line equivalent to the simpler
for bdep in pkg.spec.dependencies(deptype='build'):
There was a problem hiding this comment.
You're right, I've added a new commit fixing this.
krafczyk
changed the title
Currently only build dep bin dirs are added to the environment. This patch adds the bin dirs of run deps as well as run deps of the build deps. This fixes situations when a build dep is a kind of script such as npm which requires a run time binary like node.
If it isn't, node can't find npm or any other installed npm packages.
|
@becker33 I've rebased and addressed your concerns. Could we get this merged? |
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
Sorry about weighing in late on this!
That being said IMO this will be good if all you do is remove the lines which add run dependencies to the build-time PATH
|
|
||
| # Add all bin directories which run dependencies require. | ||
| for rdep in pkg.spec.traverse(root=False, deptype='run'): | ||
| add_dir_to_list("%s/bin" % rdep.prefix, bin_dirs) |
There was a problem hiding this comment.
IMO if the user wants a package to be part of the path when doing an install they should mark that package as a build dependency. This may be useful later since Spack in the future will try to concretize build dependencies separately (so if a package is 'run' but not 'build' that can help it avoid conflicts).
There was a problem hiding this comment.
The problem with removing these lines is that a package's build dependencies are not made available to packages which depend on it. Example:
C^(B^A(build))
Only B will be added to 'C's' path. Now if 'A' is required for 'C' to run, it will fail.
There was a problem hiding this comment.
Sorry, if A is required for 'B' to work, then C will fail when trying to run 'B' since 'A' will not be available. (more clear description)
There was a problem hiding this comment.
If A is required for B to work post-installation, doesn't that mean it should be a run dependency, not a build dependency?
There was a problem hiding this comment.
Oh yeah that's true.. Hmm I remember adding those lines because I was having a problem with NPM, maybe I was actually having a problem with my node-js and npm package. I'll take another look this weekend.
There was a problem hiding this comment.
If C needs A to run, IMO C should specify a run dependency on C directly (this is probably restating what was already said)
Sorry, if A is required for 'B' to work, then C will fail when trying to run 'B' since 'A' will not be available.
You added logic earlier (specifically line 339) which adds the transitive run dependencies of any build dependencies, so your earlier logic should handle the building of C if
C -[b]-> B -[r]-> A
There was a problem hiding this comment.
As far as setting up the run-time environment I can see that transitive run dependencies should be included, for example:
X -[r]-> Y -[r]-> Z
Y and Z should be made visible for X at run-time. However, I figure that is not the concern of this function ( as it is called "set_build_environment_variables"). If this function is being used elsewhere to set up run-time environment and I'm overlooking that let me know.
|
I would encourage us to look at #3768 and try to figure out these issues once and for all. |
|
Hey folks, sorry for the long delay. I've taken a second look at this problem with the current develop, and I'm now unable to reproduce the problem. @certik Could you try to install |
|
I checked out this PR and installed using: and it finished. I then tried to run it: and it failed. How do you actually run it? |
|
@certik, Can you please try it again, but without this PR? Try cloning a new spack version and using that to install If the install is successful, you need to do two things to make use of packages installed in spack,
I'm trying to make this process easier with the |
|
Ok, I checked the latest master and did: Let's say it works, as it did with this PR. How do I actually load it? I already have the modules installed. Can you please post the exact modules that are needed to be loaded, so that the following works: That would be very helpful. |
|
Oops, I forgot that part, To load and use py-jupyter-notebook, you need to do the following assuming you're using bash or zsh and This will load py-jupyter-notebook along with all it's dependencies. You should then be able to run I'm concerned that you may not have updated spack properly because the current |
|
I switched to the latest develop branch, and got: I will now try to clone spack to a fresh directory and redo this. |
|
@certik, You can either direct that output to a file, and then source that file like so: Or, you can take advantage of bash/zsh's process substitution to avoid creating a file like so: There are more ways of doing this as mentioned here, but process substitution is my favorite. |
|
@krafczyk I see. So with the clean develop branch, I saved the output to a file and sourced it. Now it works! Jupyter notebook loads and runs. So I think that the original problem is now fixed. Thanks for the help! |
|
This is great news! I'm waiting to hear back from @mikkokotila, but if they can run |
|
Closing, as |
8 participants
Fixes #3345.