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

configure CP2K with correct DATA_DIR value rather than relying on $CP2K_DATA_DIR environment variable #1650

Merged
merged 3 commits into from
Mar 12, 2019

Conversation

boegel
Copy link
Member

@boegel boegel commented Mar 11, 2019

Relying on $CP2K_DATA_DIR is problematic in some contexts; for example, it requires that the MPI processes also run in an environment where $CP2K_DATA_DIR is (still) defines.

Change made on recommendation of @dev-zero (one of the CP2K core developers).

(see also cp2k/cp2k#234)

datadir = os.path.join(self.installdir, 'data')
if os.path.exists(datadir):
txt += self.module_generator.set_environment('CP2K_DATA_DIR', datadir)
return txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this might be bad for users that's gotten used to being able to copy files using that env var.
Not sure if this is really a problem but it doesn't really hurt to keep it, does it?

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I was told CP2K first looks in the current directory, so unsetting this environment variable shouldn't matter, it shouldn't break existing setups.

The search order is: i) current directory, ii) configured location in installation, iii) $CP2K_DATA_DIR (not sure about the order of ii) vs iii)).

Note that no longer setting $CP2K_DATA_DIR also doesn't prevent users from using data files that are parked something else, since in that case they were probably redefining $CP2K_DATA_DIR, which still works if $CP2K_DATA_DIR is not defined.

I guess the only potential impact could be if users were used to use $CP2K_DATA_DIR to find the data directory, that will no longer work (they'd have to use $EBROOTCP2K/data instead, which wouldn't be mentioned in the CP2K documentation), so that would be a good reason to keep it anyway.

@dev-zero: Any downsides to both configuring the location of the data directory correctly via DATA_DIR in the Makefile and setting $CP2K_DATA_DIR?

Choose a reason for hiding this comment

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

This should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Defining $CP2K_DATA_DIR environment variable restored in 210f506

@wpoely86 wpoely86 merged commit 6b87b47 into easybuilders:develop Mar 12, 2019
@boegel boegel deleted the cp2k_fix_data_dir branch March 12, 2019 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants