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.

Sign up for GitHub

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

Update package.py #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update package.py #20

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/wire-cell-toolkit/package.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,5 +218,8 @@ def install(self, spec, prefix):
python(*ins)

def setup_run_environment(self, env):
env.prepend_path("WIRECELL_PATH", self.prefix.share.wirecell);
env.prepend_path("WIRECELL_PATH", self.prefix.share.wirecell)
Copy link
Member

Choose a reason for hiding this comment

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

Setting WIRECELL_PATH by default is a bit controversial. Here's the issue:

Most users/experiments maintain their configs separate from their WCT release. They include the subset of top level files that get installed into share/wirecell/. Setting WIRECELL_PATH here is incomplete and then the user must make sure to prepend their real cfg area to assure any updates that they provide take precedence over those in share/wirecell/. If they instead append, they can get hard to find bugs.

I know FNAL UPS packages do set this env var. I think that is also a net, if a small, badness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that is good information and I didn't know that was an issue. I wasn't intending to change the behavior of WIRECELL_PATH in this line, I was just removing a semicolon which I thought was unnecessary. Maybe I am underestimating the role of that semi-colon in the problem you described?

Copy link
Member

Choose a reason for hiding this comment

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

Oh! Duh, sorry. I misread the diff. Yeah, the semicolon is wrong, if only cosmetically.

I suggest we let this PR stew for a while and if we find out there is some benefit from adjusting the LD_LIBRARY_PATH we can revive things.

if 'cuda' in self.spec:
env.prepend_path("LD_LIBRARY_PATH", self.spec["cuda"].prefix.lib64)
Comment on lines +222 to +223
Copy link
Member

Choose a reason for hiding this comment

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

Setting LD_LIBRARY_PATH like this should not be required.

libcudart.so is linked with an rpath so it is found.

I believe it then locates libcuda.so and others via dynamic loading.

In particular libcuda.so is not provided by Spack (beyond the dummy "stub" instance). Rather this lib is taken from the system. On Debian, the system libcuda.so gets found with no special help.

If libcuda.so is provided at "weird" location then LD_LIBRARY_PATH would be needed. Or, likewise if the user is relocating the Spack install area then a/many LD_LIBRARY_PATH settings would be needed.

But in both cases the .prefix value would not supply the appropriate location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so this may have only been an issue on our system on Polaris. It may be the case that it is stored in a weird location. If this is only an issue on our systems, than I agree it should not be necessary. I will poke around a bit and follow up. Thanks!