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

Jump to bottom

new OpenFOAM-1612, OpenFOAM-1706 versions #4814

Closed
wants to merge 1 commit into from
Closed

new OpenFOAM-1612, OpenFOAM-1706 versions #4814

wants to merge 1 commit into from

Conversation

olesenm
Copy link

@olesenm olesenm commented Jul 5, 2017 •
edited by boegel

@olesenm olesenm force-pushed the openfoam-com branch 4 times, most recently from 85929a0 to 9878cee Compare July 11, 2017 10:38
@olesenm
Copy link
Author

olesenm commented Jul 11, 2017

Now adjusted package names too.

@olesenm
Copy link
Author

olesenm commented Jul 11, 2017

Hello @easybuilders/easybuild-easyconfigs-maintainers - review requested

@wpoely86 wpoely86 added this to the 3.4.0 milestone Jul 11, 2017
wpoely86
wpoely86 reviewed Jul 11, 2017
View reviewed changes
easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1612+-foss-2016a.eb Outdated

dependencies = [
# > causes bash issue? ('libreadline', '6.3'),
# > causes bash issue? ('ncurses', '6.0'),
Copy link
Member

Choose a reason for hiding this comment

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

please explain?

Copy link
Author

Choose a reason for hiding this comment

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

I experienced an issue where the LD_LIBRARY_PATH pointed to the easybuild libreadline, which provoked symbol conflicts for my bash.

Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on that a bit, exact error messages, etc.?

These dependencies should not be commented out, since otherwise we're at the mercy of whatever the OS provides (or doesn't), which is something we're trying to avoid.

easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1706-cleanup.patch
@@ -0,0 +1,406 @@
--- OpenFOAM-v1706.orig/applications/utilities/mesh/manipulation/setSet/Allwmake 2017-06-30 10:50:14.000000000 +0200
Copy link
Member

Choose a reason for hiding this comment

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

it's good practice to claim authorship of the patches and explain what they are supposed to do.

Copy link
Author

@olesenm olesenm Jul 12, 2017 •
edited

Choose a reason for hiding this comment

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

Where do the explanations go? I've seen some patches with # comments, but this seems to be fairly non-standard.

Copy link
Member

Choose a reason for hiding this comment

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

see my comment in the other patch

easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1706-foss-2016a.eb Outdated
and owner of the OPENFOAM trademark.
"""

toolchain = {'name': 'foss', 'version': '2016a'}
Copy link
Member

Choose a reason for hiding this comment

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

any reason why you add them for both 2016a and 2016b? Is there any difference?

Copy link
Author

Choose a reason for hiding this comment

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

Honestly, I have no idea. I'm very new to easybuild but I understood that the configs need to specify a toolchain. The 2016b toolchang uses updated gcc etc.

Copy link
Member

Choose a reason for hiding this comment

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

There's little point in providing easyconfigs for an older toolchain like foss/2016a imho.

If people really need to install OpenFOAM with it, they can use --try-toolchain.

Generally speaking, it's better to stick to more recent toolchains, so I'd prefer just having easyconfigs for foss/2016b here.

@olesenm
Copy link
Author

olesenm commented Aug 9, 2017 •
edited

What is still needed to get this restarted? @easybuilders/easybuild-easyconfigs-maintainers @easybuild-easyconfigs-maintainers

boegel
boegel requested changes Aug 10, 2017
View reviewed changes
easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1612+-cleanup.patch Outdated
@@ -0,0 +1,479 @@
--- OpenFOAM-v1612+.orig/applications/utilities/mesh/manipulation/setSet/Allwmake 2016-12-23 15:22:59.000000000 +0100
Copy link
Member

Choose a reason for hiding this comment

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

please include a brief description of the patch above this line and mention the author

any lines above this line are comments

easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1706-cleanup.patch
@@ -0,0 +1,406 @@
--- OpenFOAM-v1706.orig/applications/utilities/mesh/manipulation/setSet/Allwmake 2017-06-30 10:50:14.000000000 +0200
Copy link
Member

Choose a reason for hiding this comment

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

see my comment in the other patch

easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1706-foss-2016a.eb Outdated
and owner of the OPENFOAM trademark.
"""

toolchain = {'name': 'foss', 'version': '2016a'}
Copy link
Member

Choose a reason for hiding this comment

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

There's little point in providing easyconfigs for an older toolchain like foss/2016a imho.

If people really need to install OpenFOAM with it, they can use --try-toolchain.

Generally speaking, it's better to stick to more recent toolchains, so I'd prefer just having easyconfigs for foss/2016b here.

easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1612+-foss-2016a.eb Outdated

dependencies = [
# > causes bash issue? ('libreadline', '6.3'),
# > causes bash issue? ('ncurses', '6.0'),
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on that a bit, exact error messages, etc.?

These dependencies should not be commented out, since otherwise we're at the mercy of whatever the OS provides (or doesn't), which is something we're trying to avoid.

easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1706-foss-2016b.eb
]

sources = [ 'OpenFOAM-%(version)s.tgz' ]
checksums = ['630d30770f7b54d6809efbf94b7d7c8f']
Copy link
Member

Choose a reason for hiding this comment

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

please use SHA256 checksums (see also #5005)

easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1706-foss-2016b.eb Outdated
# Could build with CGAL, but this adds an extra python dependency
# that we don't want/need
# ('CGAL', '1.60.0'),
# ('ParaView', '5.4.0', '-mpi'),
Copy link
Member

Choose a reason for hiding this comment

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

We usually install OpenFOAM with CGAL & ParaView included as dependencies, so I'd prefer to stick to that.

What's the problem with the extra (indirect) Python dependency?

Copy link
Author

Choose a reason for hiding this comment

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

If I just want OpenFOAM, why a huge slew of python things? With paraview it would likely be good to distinguish between an off-screen and hardware mode too. However, I didn't want to get into this. All I wanted was to get a new version of OpenFOAM building with easybuild.

Copy link
Member

Choose a reason for hiding this comment

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

It's OK if you're not up for looking into this, it's just that our previous OpenFOAM easyconfigs do include CGAL & ParaView, so this is going to surprise people potentially...

easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1706-foss-2016b.eb
source_urls = [
('https://sourceforge.net/projects/openfoamplus/files/%(version)s',
'download')
]
Copy link
Member

Choose a reason for hiding this comment

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

style nitpicking: let's just make this a single line? I don't see the need to spread this over 4 lines...

source_urls = [('https://sourceforge.net/projects/openfoamplus/files/%(version)s', 'download')]

Copy link
Author

Choose a reason for hiding this comment

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

OK. On spack they run flake8 tests that complain when lines exceed 80 chars.

Copy link
Member

Choose a reason for hiding this comment

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

We have automated style checks in place too now, but allow lines up to 120 chars; too much silly splits (like this), we're not in the 80s anymore so 120 chars makes more sense imho ;)

easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1706-foss-2016b.eb Outdated
# OpenFOAM requires 64 bit METIS using 32 bit indexes (array indexes)
('METIS', '5.1.0'),
('SCOTCH', '6.0.4'),
('Boost', '1.61.0'),
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be bumped to a more recent version of Boost (e.g. 1.64.0)?

easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1706-foss-2016b.eb Outdated
]

builddependencies = [
('CMake', '3.6.1'),
Copy link
Member

Choose a reason for hiding this comment

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

this can be bumped to 3.8.2, there's an easyconfig available already for it

easybuild/easyconfigs/o/OpenFOAM/OpenFOAM-v1706-foss-2016b.eb Outdated

builddependencies = [
('CMake', '3.6.1'),
('flex', '2.6.0'),
Copy link
Member

Choose a reason for hiding this comment

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

this can be bumped to 2.6.4

Copy link
Author

Choose a reason for hiding this comment

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

probably a holdover from before. I think that 2.6.1-2.6.3 were broken (weird namespaces if I remember).

Copy link
Member

Choose a reason for hiding this comment

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

OK, but I haven't seen any issues with 2.6.4 as far as I can recall, so let's bump it?

@akesandgren
Copy link
Contributor

There is a bit of problem with OpenFOAM since there are (at least) 2 different distributions, openfoam.com and openfoam.org. And the version numbering differs.

Does anyone have any good insights on this subject?

Will the easyblock work correctly with both distributions?

@boegel
Copy link
Member

boegel commented Sep 6, 2017

@akesandgren Some updates are done in easybuilders/easybuild-easyblocks#1205, but for now it seems like the OpenFOAM easyblock is able to copy with both flavours (+ OpenFOAM-Extend as well)...

@boegel boegel modified the milestones: 3.5.0, 3.4.0 Sep 6, 2017
@easybuilders easybuilders deleted a comment from boegelbot Sep 6, 2017
@easybuilders easybuilders deleted a comment from boegelbot Sep 6, 2017
@boegel
Copy link
Member

boegel commented Oct 17, 2017

@olesenm Are you up for following up on this, or do you prefer that we take it from here?

@olesenm
Copy link
Author

olesenm commented Oct 18, 2017

I'm fine with whatever works best for you (ie, please take over if possible). In a longer term I think that the various flavour/fork check should really only been done once, but that is just a style issue. At least for openfoam, I think that the changes to its config files would be better to do directly (if possible) rather that using patching to modify things. I've used this with reasonably good success in spack. When building openfoam-1706 with spack, I only use patch to adjust two entries in a single file and the rest of the configuration is simply emitted directly.

@boegel boegel mentioned this pull request Oct 19, 2017
Merged
@boegel boegel modified the milestones: 3.5.0, next release Dec 6, 2017
@olesenm
Copy link
Author

olesenm commented Dec 11, 2017

I've picked this up again (would be nice to get it past the line before the 1712 release). However, I'm currently going crazy with all sorts of foss-2016b checksum errors. The tarfiles in question (Autoconf, libtool, M4, numactl) all have sha256sums that match what their easybuild files require, but I always get errors. Must be some flags somewhere I guess.

@boegel
Copy link
Member

boegel commented Dec 12, 2017

@olesenm Can you share which exact errors you are getting, and how you're triggering them? Can you also share your EasyBuild configuration (output of eb --show-config should suffice).

@olesenm
Copy link
Author

olesenm commented Dec 13, 2017

Here is a rebuild log of one of the offenders (m4):
easybuild-oIFDJq.log

A manual verification of the check sums from the command line

checksum result
md5sum a5e9954b1dae036762f7b13673a2cf76 /home/mol/.local/easybuild/sources/m/M4/m4-1.4.17.tar.gz
sha256sum 3ce725133ee552b8b4baca7837fb772940b25e81b2a9dc92537aeaf733538c9e /home/mol/.local/easybuild/sources/m/M4/m4-1.4.17.tar.gz

So the sha256sum looks correct, but it would appear that verify_checksum still doesn't like it.

The easybuild config (eb --show-config):

#
# Current EasyBuild configuration
# (C: command line argument, D: default value, E: environment variable, F: configuration file)
#
buildpath      (D) = /home/mol/.local/easybuild/build
installpath    (D) = /home/mol/.local/easybuild
module-syntax  (E) = Tcl
modules-tool   (E) = EnvironmentModulesC
repositorypath (D) = /home/mol/.local/easybuild/ebfiles_repo
robot-paths    (D) = /home/mol/Software/easybuild/easybuild-easyconfigs/easybuild/easyconfigs
sourcepath     (D) = /home/mol/.local/easybuild/sources

The easyconfig is develop at 4a61927
The easyblocks is develop at 96efeb6eda2fd6c1abcdb596fdcc75d833ab5c4e.

@olesenm olesenm force-pushed the openfoam-com branch 2 times, most recently from 7f43b2c to ec7ea90 Compare December 13, 2017 08:00
@boegelbot
Copy link
Collaborator

Travis test report: 8/8 runs failed - see https://travis-ci.org/easybuilders/easybuild-easyconfigs/builds/315762446

Only showing partial log for 1st failed test suite run 7180.1;
full log at https://travis-ci.org/easybuilders/easybuild-easyconfigs/jobs/315762449

...
FAIL: Check the easyconfigs for style
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/easybuilders/easybuild-easyconfigs/test/easyconfigs/styletests.py", line 61, in test_style_conformance
    self.assertEqual(result, 0, "Found code style errors (and/or warnings): %s" % result)
AssertionError: Found code style errors (and/or warnings): 4

----------------------------------------------------------------------
Ran 8106 tests in 406.462s

FAILED (failures=1)
ERROR: Not all tests were successful.

(bleep, bloop, I'm just a bot, please talk to my owner @boegel if you notice you me acting stupid)

@boegel boegel removed this from the 3.5.1 milestone Jan 11, 2018
@boegel boegel added this to the 3.6.0 milestone Jan 11, 2018
@easybuilders easybuilders deleted a comment from boegelbot Jan 26, 2018
@boegel
Copy link
Member

boegel commented Feb 24, 2018

@olesenm Is this still relevant now that #5781 is merged which adds easyconfigs for OpenFOAM v1712?

@boegel boegel modified the milestones: 3.5.2, 3.x Feb 24, 2018
@olesenm
Copy link
Author

olesenm commented Feb 28, 2018

I think that this content has probably been merged, so I'll close it and see it that is true.

@olesenm olesenm closed this Feb 28, 2018
@boegel
Copy link
Member

boegel commented Feb 28, 2018

@olesenm We have only merged easyconfigs for OpenFOAM v1712, not for the older versions you were adding here, but that's good enough going forward?

@olesenm
Copy link
Author

olesenm commented Feb 28, 2018

Sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wpoely86 wpoely86 wpoely86 left review comments

@boegel boegel boegel requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.x
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@olesenm @akesandgren @boegel @boegelbot @wpoely86