Skip to content

add easyblock for CFDEMcoupling#1439

Merged
akesandgren merged 5 commits intoeasybuilders:developfrom
boegel:CFDEMcoupling
Oct 16, 2020
Merged

add easyblock for CFDEMcoupling#1439
akesandgren merged 5 commits intoeasybuilders:developfrom
boegel:CFDEMcoupling

Conversation

@boegel
Copy link
Copy Markdown
Member

@boegel boegel commented Jun 15, 2018

No description provided.

Comment thread easybuild/easyblocks/c/cfdemcoupling.py Outdated
orig_dir = os.path.join(self.builddir, pkg_topdirs[0])
move_file(orig_dir, target_dir)
else:
raise EasyBuildError("Failed to find subdirectory for %s in %s %s", pkgname, self.builddir, top_dirs)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe suggest here that the sources for that package might be missing? If LIGGGTHS is missing easybuild will proceed until this point and then say that the directory hasn't been found, but giving no clue about the reason.

Comment thread easybuild/easyblocks/c/cfdemcoupling.py Outdated
else:
raise EasyBuildError("Failed to find subdirectory for %s in %s %s", pkgname, self.builddir, top_dirs)

# always use env.setvar instead of os.putenv or os.environ for defining environment variables
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems more like a guideline for easyblock developers than a relevant comment for this particular easyblock. I'd remove it.

Comment thread easybuild/easyblocks/c/cfdemcoupling.py Outdated
env.setvar('CFDEM_PROJECT_DIR', self.cfdem_project_dir)

# define $CFDEM_PROJECT_USER_DIR to an empty existing directory
env.setvar('CFDEM_PROJECT_USER_DIR', os.path.join(self.builddir, 'CFDEM_PROJECT_USER_DIR'))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CFDEM_PROJECT_USER_DIR seems to me like a weird name for a directory, and it is confusing that the directory name is the same as the environment variable. I suggest to rename it to user_dir or something similar.

Comment thread easybuild/easyblocks/c/cfdemcoupling.py Outdated

# define $CFDEM_PROJECT_USER_DIR to an empty existing directory
env.setvar('CFDEM_PROJECT_USER_DIR', os.path.join(self.builddir, 'CFDEM_PROJECT_USER_DIR'))
mkdir(os.getenv('CFDEM_PROJECT_USER_DIR'), parents=True)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems odd to set a variable and read it right after. Not that it matters a lot, but it is certainly not efficient. Doesn't it make more sense to define a python variable and use it in both places?

Comment thread easybuild/easyblocks/c/cfdemcoupling.py Outdated
vtk_root = get_software_root('VTK')
if vtk_root:
vtk_ver_maj_min = '.'.join(get_software_version('VTK').split('.')[:2])
env.setvar('VTK_INC_USR', '-I%s' % os.path.join(vtk_root, 'include', 'vtk-%s' % vtk_ver_maj_min))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it makes sense to check if these directories exists and are not empty.

Comment thread easybuild/easyblocks/c/cfdemcoupling.py Outdated
else:
raise EasyBuildError("VTK not included as dependency")

# can't seem to use defined 'cfdemSysTest' alias...
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not sure I understand the meaning of this comment.......

@boegel boegel modified the milestones: 3.6.2, next release Jul 5, 2018
@damianam
Copy link
Copy Markdown
Member

@boegel ping

@boegel boegel modified the milestones: 3.x, 4.x Dec 3, 2019
@akesandgren
Copy link
Copy Markdown
Contributor

@boegel ping since this blocks EC PR 6465 which is in the sprint...

Comment thread easybuild/easyblocks/c/cfdemcoupling.py
Comment thread easybuild/easyblocks/c/cfdemcoupling.py
Copy link
Copy Markdown
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

@akesandgren
Copy link
Copy Markdown
Contributor

Going in, thanks @boegel!

@akesandgren akesandgren merged commit ba184a6 into easybuilders:develop Oct 16, 2020
@boegel boegel deleted the CFDEMcoupling branch October 16, 2020 18:28
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.

4 participants