don't hardcode .so, use get_shared_lib_ext instead by citibeth · Pull Request #789 · easybuilders/easybuild-easyblocks

citibeth · 2016-01-02T13:59:06Z

Repeat of .dylib fix

(edit: same changes as in #788, but to develop)

hpcugentbot · 2016-01-02T13:59:07Z

Automatic reply from Jenkins: Can I test this?

boegel · 2016-01-02T16:11:40Z

'constants' (i.e., variables with a name in capitals) are only defined outside of classes/functions in the coding style we maintain, so please change this to:

shlib_ext = get_shared_lib_ext()

boegel · 2016-01-02T16:17:06Z

@citibob: the proposed changes look fine, just some small remarks w.r.t. code style; maintaining a uniform coding style across the entire project really helps in lowering the bar for contributions, so please take them into account and apply the suggestions consistently

I'll also trigger our bot to run the unit tests, but I don't expect any problems there (for easyblocks, it mostly checks for silly syntax errors, it doesn't retest using the easyblock itself)

Don't hesitate to let us know if you have any questions!

boegel · 2016-01-02T16:17:11Z

Jenkins: ok to test

boegel · 2016-01-02T16:19:04Z

@citibob: I also tweaked the title of your PR, that's going to make it a lot easier to track (and for me to include it in the release notes for the next EB release)

hpcugentbot · 2016-01-02T16:20:32Z

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1542/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

citibeth · 2016-01-02T16:24:23Z

Thank you for the input. At this point... do I need to make the edits, or
have they already been made? If I do need to make them, what do I need to
do on GitHub to re-submit? This is my first pull request...

On Sat, Jan 2, 2016 at 11:17 AM, Kenneth Hoste [email protected]
wrote:

@citibob https://github.com/citibob: the proposed changes look fine,
just some small remarks w.r.t. code style; maintaining a uniform coding
style across the entire project really helps in lowering the bar for
contributions, so please take them into account and apply the suggestions
consistently

I'll also trigger our bot to run the unit tests, but I don't expect any
problems there (for easyblocks, it mostly checks for silly syntax errors,
it doesn't retest using the easyblock itself)

Don't hesitate to let us know if you have any questions!

—
Reply to this email directly or view it on GitHub
#789 (comment)
.

boegel · 2016-01-02T16:46:10Z

@citibob: no, the changes have not been made, I just added some in-line remarks in the pull request.
We're hoping that you're up for tackling the suggested changes yourself (if you're not, just let us know).

The easiest way to follow-up on them is to take a look at the 'diff' view on GitHub, which you can reach via the 'Files changed' tab (https://github.com/hpcugent/easybuild-easyblocks/pull/789/files).

That will give you an overview of the suggestions I made, alongside the lines of code to which the suggestions apply.

After making the changes, just include them in your 160101-Dylib-dev branch in an additional commit, and push your branch to your GitHub fork of this repo again, which will update the pull request.

More or less this:

git checkout 160101-Dylib-dev
$EDITOR ...
git commit -a -m "fix style remarks"
git push ...

Don't hesitate to let us know if you have any questions!

citibeth · 2016-01-02T20:39:10Z

OK, should be ready now for new review. Hopefully I haven't broken
anything, I haven't re-run the builds.

More generally... I'm thinking of trying an automated search-and-replace
for the so/dylib issue. It's really almost the only thing causing problem
on the Mac (other than package-specific Mac problems).

I figure it can't hurt to try. I'll do an automated fix, submit a pull
request, and then we can see if we like it.

-- Elizabeth

On Sat, Jan 2, 2016 at 11:46 AM, Kenneth Hoste [email protected]
wrote:

@citibob https://github.com/citibob: no, the changes have not been
made, I just added some in-line remarks in the pull request.
We're hoping that you're up for tackling the suggested changes yourself
(if you're not, just let us know).

The easiest way to follow-up on them is to take a look at the 'diff' view
on GitHub, which you can reach via the 'Files changed' tab (
https://github.com/hpcugent/easybuild-easyblocks/pull/789/files).

That will give you an overview of the suggestions I made, alongside the
lines of code to which the suggestions apply.

After making the changes, just include them in your 160101-Dylib-dev
branch in an additional commit, and push your branch to your GitHub fork of
this repo again, which will update the pull request.

More or less this:

git checkout 160101-Dylib-dev
$EDITOR ...
git commit -a -m "fix style remarks"
git push ...

Don't hesitate to let us know if you have any questions!

—
Reply to this email directly or view it on GitHub
#789 (comment)
.

…ased on: grep '\.so' `find . -name '*.py'` | less The changes have NOT been tested. Some files were not changed for the following reasons: atlas.py: Spurious .so turned up by grep cblas.py, openssl.py: The easyblock looks for libraries like libx.so.1.1.1 --- which translate on the Mac as libx.1.1.1.dylib. This will require a little more effort to get right. geant4.py: The easyblock has a build conversation with the package. It looks for prompts containing ".so". Without further inspection, it's hard to know what those prompts will be like on the Mac; hence, this file was not touched. numpy.py: .so was only in comments. Furthermore... it is referring to a Python extension, which retains the same '.so' extension on Mac and Linux. petsc.py: The '.so' here refers to a system-standard '/usr/lib64/libpapi.so', rather than a shared library created by the project. There might be other issues with this on a Mac, so it has not been touched for now. NOTE: The following Emacs-Lisp commands were evaluated to ease this process. (global-set-key (kbd "C-1") "from easybuild.tools.systemtools import get_shared_lib_ext\n") (global-set-key (kbd "C-2") " shlib_ext = get_shared_lib_ext()\n") (global-set-key (kbd "C-3") "shlib_ext")

boegel · 2016-01-02T22:00:21Z

@citibob: no extra commit was pushed to the branch that corresponds with this PR? Do you have the commit ID?

citibeth · 2016-01-02T22:47:01Z

Sorry, git confusion. It should be there now.

The latest commit is SHA 27d32de. I "fixed" (almost) all the easyblocks.

-- Elizabeth

On Sat, Jan 2, 2016 at 5:00 PM, Kenneth Hoste [email protected]
wrote:

@citibob https://github.com/citibob: no extra commit was pushed to the
branch that corresponds with this PR? Do you have the commit ID?

—
Reply to this email directly or view it on GitHub
#789 (comment)
.

hpcugentbot · 2016-01-02T22:47:47Z

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1543/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

boegel · 2016-01-03T16:34:52Z

@citibob: looks great! I gave your proposed changes a thorough review, and couldn't spot any problems.

A couple of style issues came to my attention though, but since most a not caused/related to your changes, I'll tackle those in a follow-up PR, rather than block this PR over it.

Going in, thanks a lot for looking into this!

And congratulations on your first (code) contribution to EasyBuild, hopefully one of many more to come.

don't hardcode .so, use get_shared_lib_ext instead

boegel · 2016-01-03T17:03:51Z

fwiw: while testing my follow-up PR #790, I ran into a bug introduced here;

shlib_ext is defined as so, not .so, so I changed it to use a here and .%s in the line below, see #790

bug fix in ACML easyblock and minor style fixes after #789 (1/4)

minor style fixes after #789 (2/4)

minor style fixes after #789 (4/4)

minor style fixes after #789 (3/4)

fix typo in imkl.py (introduced in #789)

rpfische added 2 commits January 2, 2016 00:27

Fixed hardcoded .so in two places (tested too).

53b03e1

Another .so fix

cb78f8c

boegel mentioned this pull request Jan 2, 2016

160101 dylib dev: Some fixes for Macintosh #788

Closed

boegel added this to the v2.6.0 milestone Jan 2, 2016

boegel reviewed Jan 2, 2016
View reviewed changes

boegel changed the title ~~160101 dylib dev~~ don't hardcode .so in HDF5 and netCDF* easyblocks Jan 2, 2016

boegel mentioned this pull request Jan 2, 2016

Make it work on Macintosh #787

Open

Stylistic changes to pull request.5

ca6afe8

boegel changed the title ~~don't hardcode .so in HDF5 and netCDF* easyblocks~~ don't hardcode .so, use get_shared_lib_ext instead Jan 3, 2016

boegel added a commit that referenced this pull request Jan 3, 2016

Merge pull request #789 from citibob/160101-Dylib-dev

db58074

don't hardcode .so, use get_shared_lib_ext instead

boegel merged commit db58074 into easybuilders:develop Jan 3, 2016

boegel mentioned this pull request Jan 3, 2016

bug fix in ACML easyblock and minor style fixes after #789 (1/4) #790

Merged

boegel reviewed Jan 3, 2016
View reviewed changes

boegel added a commit that referenced this pull request Jan 4, 2016

Merge pull request #790 from boegel/style_fixes_shlib_a_c

a29111a

bug fix in ACML easyblock and minor style fixes after #789 (1/4)

This was referenced Jan 4, 2016

minor style fixes after #789 (2/4) #791

Merged

minor style fixes after #789 (3/4) #793

Merged

minor style fixes after #789 (4/4) #794

Merged

boegel added a commit that referenced this pull request Jan 4, 2016

Merge pull request #791 from boegel/style_fixes_e2i

039df5a

minor style fixes after #789 (2/4)

boegel added a commit that referenced this pull request Jan 4, 2016

Merge pull request #794 from boegel/style_fixes_o2u

5b693af

minor style fixes after #789 (4/4)

boegel added a commit that referenced this pull request Jan 4, 2016

Merge pull request #793 from boegel/style_fixes_l2n

db456c9

minor style fixes after #789 (3/4)

boegel added a commit to boegel/easybuild-easyblocks that referenced this pull request Jan 11, 2016

fix typo in imkl.py (introduced in easybuilders#789)

8dc28af

boegel added a commit that referenced this pull request Jan 11, 2016

Merge pull request #803 from boegel/fix_typo

a5e085c

fix typo in imkl.py (introduced in #789)

Conversation

citibeth commented Jan 2, 2016

Uh oh!

hpcugentbot commented Jan 2, 2016

Uh oh!

boegel Jan 2, 2016

Choose a reason for hiding this comment

Uh oh!

boegel commented Jan 2, 2016

Uh oh!

boegel commented Jan 2, 2016

Uh oh!

boegel commented Jan 2, 2016

Uh oh!

hpcugentbot commented Jan 2, 2016

Uh oh!

citibeth commented Jan 2, 2016

Uh oh!

boegel commented Jan 2, 2016

Uh oh!

citibeth commented Jan 2, 2016

Uh oh!

boegel commented Jan 2, 2016

Uh oh!

citibeth commented Jan 2, 2016

Uh oh!

hpcugentbot commented Jan 2, 2016

Uh oh!

boegel commented Jan 3, 2016

Uh oh!

boegel Jan 3, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants