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

only add useful entries for $CPATH, $(LD_)LIBRARY_PATH and $PATH #3145

Merged
merged 3 commits into from
Dec 30, 2019

Conversation

bartoldeman
Copy link
Contributor

This avoids adding useless entries in the environment, an
example being GCCcore which adds lib and include which are both
empty apart from subdirectories.

This avoids adding useless entries in the environment, an
example being GCCcore which adds lib and include which are both
empty apart from subdirectories.
@boegel boegel added the bug fix label Dec 28, 2019
@boegel boegel added this to the next release (4.1.1) milestone Dec 28, 2019
@boegel boegel changed the title For PATH, CPATH, {LD_,}LIBRARY_PATH only add useful entries. only add useful entries for $CPATH, $(LD_)LIBRARY_PATH and $PATH Dec 28, 2019
@@ -1301,6 +1301,12 @@ def make_module_req(self):
# only use glob if the string is non-empty
if path and not self.dry_run:
paths = sorted(glob.glob(path))
if key in ('PATH', 'CPATH', 'LIBRARY_PATH', 'LD_LIBRARY_PATH') and len(paths) == 1:
Copy link
Member

Choose a reason for hiding this comment

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

@bartoldeman I don't think there's a need to only do this when there's a single directory being considered?

see ComputeCanada#18

also only retain subdirs with at least one file in case there are multiple paths considered in make_module_req
@boegel boegel merged commit c742f59 into easybuilders:develop Dec 30, 2019
@Flamefire
Copy link
Contributor

This heavily breaks modules!

You usually have a structure like include/libfoo/libfoo.h but include won't be added to CPATH due to it not containing a file, but "only" a folder

Working on a fix...

@bartoldeman
Copy link
Contributor Author

@Flamefire correct, sorry I missed that. May be simplest to exclude CPATH? The same reasoning does not apply to LIBRARY_PATH/LD_LIBRARY_PATH/PATH.

@Flamefire
Copy link
Contributor

Does it? I think you are right, but I'm not 100% sure. E.g. pkgconfig for a header-only library would be a counter-example.

@Flamefire
Copy link
Contributor

Playing it safe: #3152 removes only folders that have no files recursively

@bartoldeman
Copy link
Contributor Author

Can you elaborate on pkgconfig @Flamefire ? PKG_CONFIG_PATH is not in the list so I don't understand this.

@bartoldeman
Copy link
Contributor Author

Now the safe fix defeats the purpose which is to avoid the empty GCCcore directories and similar cases, now they are added again.

@Flamefire
Copy link
Contributor

Ah yes, you are right. Still: Are you 100% sure there is no lib/somefolder/somefile which is used by sometool if lib is in (LD_)LIBRARY_PATH? I'm not.

Can you elaborate on the GCCcore issue? If the folders are really empty, they won't be added. "Empty"=="Do not contain any file and no subdirectory which contains a file" so lib/foo/bar would not be added unless there is a file in bar

@boegel
Copy link
Member

boegel commented Jan 7, 2020

I don't understand @bartoldeman's point either, the fix in #3152 (ignoring directories that don't contain any files) makes sense imho...

@bartoldeman
Copy link
Contributor Author

@Flamefire @boegel With GCCcore we have:

@bartoldeman
Copy link
Contributor Author

Actually I should now change the gcc easyblock to remove lib from LD_LIBRARY_PATH for non-32-bit capable gcc (ie. the common case). I'll submit a new PR for that.

@Flamefire
Copy link
Contributor

Flamefire commented Jan 7, 2020

Ok so there is a file vector in include/c++/8.3.0, did I get this right? Generically you'd want this path added. It is GCC being an exception here because it is the compiler, but for everything else you'd need it. If you don't, you'd need to adapt that in the EasyBlock (or config)

What is the deal with lib? Is there any file in there in 64 bit? If not, my PR is enough, isn't it?

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.

3 participants