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

Deprecate adding a non-existing path to $MODULEPATH #3637

Merged
merged 10 commits into from
May 25, 2021

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Apr 7, 2021

The module.use function is not actually supposed to create a directory, but we require that the directories of $MODULEPATH exist in curr_module_paths

Hence deprecate adding a path which does not exist

As a side effect we need to explicitly disallow the empty path '', as that also does not exist. EnvironmentModules ignores such paths, LMod does create an empty entry in $MODULEPATH but doesn't really use it. We also filter it in curr_module_paths so it makes sense to fully disallow adding it.

During the tests on CI another bug was discovered: The module tools (at least Lmod) strip trailing slashes before adding a path to $MODULEPATH. However we sometimes do add_module_path(os.path.join(some_prefix, modpath)) where modpath could be empty which leads to a trailing slash and add_module_path (and remove_module_path) not finding the path in curr_module_paths (which do not contain the trailing slashes) This means that paths wrongly were retained in $MODULEPATH or unnecessarily added again.

I added a test to catch this issue and fixed it.

I also had to add checks before adding Hierarchical MNS paths, such as Core if they exist (already). Not adding non-existing ones is fine as far as I can tell and even leads to more speed by avoiding module calls, cache invalidation and work for the modules tool (repeatably checking a non-existing folder)

@Flamefire Flamefire force-pushed the deprecateCreatingDirInUse branch 3 times, most recently from 29c791d to ac903ea Compare April 8, 2021 15:29
@Flamefire Flamefire force-pushed the deprecateCreatingDirInUse branch from ac903ea to 65676df Compare April 15, 2021 10:55
@easybuilders easybuilders deleted a comment from boegelbot Apr 28, 2021
@easybuilders easybuilders deleted a comment from boegelbot Apr 28, 2021
@boegel boegel added the change label Apr 28, 2021
@boegel boegel added this to the next release (4.3.5?) milestone Apr 28, 2021
test/framework/modules.py Outdated Show resolved Hide resolved
test/framework/modules.py Outdated Show resolved Hide resolved
test/framework/modules.py Outdated Show resolved Hide resolved
Copy link
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

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

LGTM
Still more places where we should do os.pathsep.join and os.path.join but let's leave that for another cleanup PR someday.

@akesandgren
Copy link
Contributor

Going in, thanks @Flamefire!

@akesandgren akesandgren merged commit dec669c into easybuilders:develop May 25, 2021
@Flamefire Flamefire deleted the deprecateCreatingDirInUse branch May 26, 2021 06:54
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