add '-Dusethreads' to perl build Configure #6

tomkinsc · 2016-09-06T15:36:14Z

Closes #5
Fixes #4 ?

The synchronization overhead of perl with threading support is minimal. Single-threaded perl programs will not see a significant performance penalty from being run on perl supporting threads (at most about 5%), however without threading support, multi-threaded programs cannot run.
It would be a sane default to include ‘-Dusethreads’ in the perl build script, so any packages that depend on ‘perl’ will run, whether they are single- or multi-threaded. It may make sense to add a second feedstock for something like ‘perl-singlethreaded’ for those who need the absolute best efficiency for their single-threaded perl applications.

This includes the flag ordering change present in #5 where -de is added at the end, since apparently the order matters.

Edit by @jakirkham : Added closes/fixes lines above.

The synchronization overhead of perl with threading support is minimal. Single-threaded perl programs will not see a significant performance penalty from being run on perl supporting threads (at most about 5%), however without threading support, multi-threaded programs cannot run. It would be a sane default to include ‘-Dusethreads’ in the perl build script, so any packages that depend on ‘perl’ will run, whether they are single- or multi-threaded. It may make sense to add a second feedstock for something like ‘perl-singlethreaded’ for those who need the absolute best efficiency for their single-threaded perl applications.

conda-forge-linter · 2016-09-06T15:36:20Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

jakirkham · 2016-09-06T16:14:35Z

recipe/build.sh

@@ -1,5 +1,5 @@
 #!/bin/bash
-sh Configure -de -Dprefix=$PREFIX -Duserelocatableinc
+sh Configure -Dusethreads -Duserelocatableinc -Dprefix=$PREFIX -de


So I believe there was something special about how these needed to be ordered. See issue ( #4 ). If you could explain a bit why this order is better, that would be very helpful.

Also please see PR ( #5 ) where I was trying to do the same thing.

Yes, I placed -de at the end for that reason (saw your pull request).

tomkinsc · 2016-09-06T16:43:40Z

It looks like the build failed with errors such as this one:

Running Makefile.PL in cpan/Pod-Checker ../../miniperl -I../../lib Makefile.PL INSTALLDIRS=perl INSTALLMAN1DIR=none INSTALLMAN3DIR=none PERL_CORE=1 LIBPERL_A=libperl.a WARNING: Setting ABSTRACT via file 'lib/Pod/Checker.pm' failed

Any ideas how to correct it?

bgruening · 2016-09-11T10:19:28Z

@tomkinsc no idea from my side :(

bgruening · 2016-11-13T20:42:34Z

@tomkinsc maybe the trick mentioned at the end of the error report helps?

### You may have to set your dynamic library search path,
### LD_LIBRARY_PATH, to point to the build directory:
###   setenv LD_LIBRARY_PATH `pwd`; cd t; ./perl harness
###   LD_LIBRARY_PATH=`pwd`; export LD_LIBRARY_PATH; cd t; ./perl harness
###   export LD_LIBRARY_PATH=`pwd`; cd t; ./perl harness
### for csh-style shells, like tcsh; or for traditional/modern
### Bourne-style shells, like bash, ksh, and zsh, respectively.

bgruening · 2016-11-13T20:52:23Z

Moreover, this Insecure directory in $ENV{PATH} while running with -T switch at ../../t/test.pl line 738. seems to indicate that a few files and folders are world-writable, which is not allowed.

jakirkham · 2016-11-14T19:59:06Z

If there are few Bioconda people interested in helping maintain Perl, that would be great. I think you guys use Perl more often than I or others do. So you probably have a better idea of how best to get it to work.

bgruening · 2016-11-14T20:02:39Z

@jakirkham we are here to help :)
@tomkinsc are the travis changes really needed?

bgruening · 2016-11-14T20:04:21Z

@tomkinsc I cancelled the previous builds so that the last one is faster :)

tomkinsc · 2016-11-14T21:05:54Z

@bgruening: Yes, the changes were needed since I had to update the CircleCI-related files, and Travis had a shared dependency. Thanks for cancelling the previous builds! :)

tomkinsc · 2016-11-14T21:42:42Z

Not sure about the AppVeyor failure.

bgruening · 2016-11-14T21:46:46Z

It seems that the appveyor config also needs some updates? Other feedstocks have a different config.
@jakirkham I thought conda-smithy is updating these configs, no?

jakirkham · 2016-11-14T21:57:25Z

So we have a webservice that re-renders a random subset of feedstocks daily. However, the number of feedstocks has really grown and that approach is proving itself insufficient for this scale. We have been discussing an asynchronous webservice that PRs re-renderings against feedstocks/PRs when they get updated. ( conda-forge/conda-forge-webservices#74 ) That should do a better job going forward.

In the near term, I have put a fair bit of effort into just making conda-smithy easier to use and documenting it better in the hopes that people can do this themselves as needed.

bgruening · 2016-11-14T22:02:48Z

@jakirkham I guess this is fair enough. We should just know that smithy is not taking care of it :)

jakirkham · 2016-11-14T22:03:15Z

Also the Windows build didn't/doesn't actually build Perl. Instead it installed a pre-built binary of Perl. This is what @msarahan found to work at the time. Hence why it didn't/shouldn't have a Python matrix on AppVeyor.

bgruening · 2016-11-14T22:06:25Z

So skip: True # [win] is the way forward? Good to know.

jakirkham · 2016-11-14T22:09:53Z

Uh, sorry, I think the wires got crossed. What I mean is there is a Windows build, but it just copies StrawberryPerl over into the conda build prefix. We just don't need the typical python build dependency or vc features.

tomkinsc · 2016-11-14T22:10:37Z

I don't have time to address this, so I'm closing this PR; feel free to fork my branch. Setting up various tokens and "re-rendering" the automation infrastructure is a silly amount of overhead when I simply wanted to add a flag to the conda build script.

conda-forge-linter · 2016-11-15T17:23:49Z

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

…d-by-default

conda-forge-linter · 2016-11-15T17:26:25Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

tomkinsc · 2016-11-15T17:27:53Z

Thanks for re-rendering the feedstock. I've merged the updated master into this branch.

jakirkham · 2016-11-15T20:02:47Z

Of course, glad to help.

It seems there are some subtle differences on some of the CI files. Could you please click the Allow edits from maintainers box and I can push a fix to clean those up directly to your PR?

tomkinsc · 2016-11-15T20:14:27Z

Done.

jakirkham · 2016-11-15T20:30:00Z

Thanks.

jakirkham · 2016-11-15T21:41:16Z

recipe/build.sh

+    old_ld_library_path="$LD_LIBRARY_PATH"
+fi
+
+export LD_LIBRARY_PATH=$(pwd)


Could you please explain why this change was needed?

Suggested by prior perl test failures.

jakirkham · 2016-11-15T21:41:49Z

recipe/build.sh

+export LD_LIBRARY_PATH=$(pwd)
+
+# world-writable files are not allowed
+chmod -R o-w $SRC_DIR


Also would be good to get a bit more knowledge on why the permissions are off.

Incorrect as built by perl?

jakirkham · 2016-11-15T21:42:04Z

recipe/build.sh

 sh Configure -de -Dprefix=$PREFIX -Duserelocatableinc
 make
+
+# change permissions again after building
+chmod -R o-w $SRC_DIR


...and how they become wrong again.

Built files may have incorrect permissions, so the script corrects before building and again after. Tests were previously failing because various files were world-writable.

jakirkham · 2016-11-15T21:42:25Z

recipe/build.sh

+
+# world-writable files are not allowed
+chmod -R o-w $SRC_DIR
+
 sh Configure -de -Dprefix=$PREFIX -Duserelocatableinc


Are we still planning to add -Dusethreads?

Not sure how it was lost in this PR, but yes.

Ok, I guess this is needed before we can ask for a merge :)

jakirkham · 2016-11-15T21:42:56Z

recipe/build.sh

+
+if [ ! -z "$old_ld_library_path" ]; then
+    export LD_LIBRARY_PATH="$old_ld_library_path"
+fi


Why do we need to set it back? Doesn't conda-build override this anyways?

If conda-build will restore it, then this can be removed.

I agree, this can be removed.

bgruening

I guess with these two changes we can ask for merging it.
Thanks @tomkinsc.

bgruening · 2016-11-19T10:40:17Z

recipe/build.sh

+
+# world-writable files are not allowed
+chmod -R o-w $SRC_DIR
+
 sh Configure -de -Dprefix=$PREFIX -Duserelocatableinc


Ok, I guess this is needed before we can ask for a merge :)

bgruening · 2016-11-19T10:41:02Z

recipe/build.sh

+
+if [ ! -z "$old_ld_library_path" ]; then
+    export LD_LIBRARY_PATH="$old_ld_library_path"
+fi


I agree, this can be removed.

bgruening · 2016-12-01T19:45:08Z

@tomkinsc do you have time to finishe this one?

conda-forge-linter · 2016-12-02T18:38:36Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

bgruening · 2016-12-03T21:38:09Z

@jakirkham ready to be merged? :)
Thanks a lot @tomkinsc!

jjhelmus · 2016-12-04T00:30:56Z

Any idea if these changes would make perl relocatable (ie fix #4)? I've been trying to re-build the conda-forge autoconf package with conda-build 2.x and something with the current perl build is causing issues.

I'll move this topic to it's own issue this is unrelated to the current changes.

jjhelmus · 2016-12-04T04:20:22Z

Please ignore my last comment. The failure I was seeing is unrelated to the perl package. Sorry for any confusion.

tomkinsc · 2016-12-04T04:21:03Z

Ready to merge!

jakirkham · 2016-12-06T18:05:04Z

recipe/build.sh

 make test
-make install
+make install


Note: Fixed to a terminal EOF again.

tomkinsc mentioned this pull request Sep 6, 2016

Gd jpeg bioconda/bioconda-recipes#2289

Closed

4 tasks

jakirkham reviewed Sep 6, 2016
View reviewed changes

tomkinsc closed this Sep 8, 2016

tomkinsc reopened this Sep 8, 2016

tomkinsc added 7 commits November 14, 2016 11:38

set LD_LIBRARY_PATH to pwd, and chmod so files are not world writable

97d106b

update ci-related files

5416975

remove .txt extension

1be009b

chmod +x ci script

2a5960d

octal to shorthand in chmod

78d66bb

increment build number

4cba6ee

update .travis.yml from newer feedstock template

6700024

update appveyor.yml

5c4222b

tomkinsc reopened this Nov 15, 2016

Merge remote-tracking branch 'conda-forge/master' into ct-perl-theade…

aadee02

…d-by-default

MNT: Re-rendered with conda-smithy 1.5.3

cb87864

build.sh: Add terminal newline.

47066d1

jakirkham reviewed Nov 15, 2016

View reviewed changes

bgruening reviewed Nov 19, 2016

View reviewed changes

add -Dusethreads to Configure call

8599a44

recipe/build.sh: Add terminal EOL.

061c57f

jakirkham reviewed Dec 6, 2016

View reviewed changes

recipe/build.sh

make test

make install

make install

Copy link

Member

jakirkham Dec 6, 2016

Choose a reason for hiding this comment

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

Note: Fixed to a terminal EOF again.

jakirkham approved these changes Dec 6, 2016

View reviewed changes

jakirkham mentioned this pull request Dec 6, 2016

perl conda package is not relocatable #4

Closed

jakirkham merged commit b957735 into conda-forge:master Dec 7, 2016

tomkinsc deleted the ct-perl-theaded-by-default branch December 8, 2016 05:41

bgruening mentioned this pull request Dec 11, 2016

perl-threaded can be deprecated bioconda/bioconda-recipes#3167

Closed

add '-Dusethreads' to perl build Configure #6

add '-Dusethreads' to perl build Configure #6

Conversation

tomkinsc commented Sep 6, 2016 • edited by jakirkham Loading

conda-forge-linter commented Sep 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomkinsc commented Sep 6, 2016 • edited Loading

bgruening commented Sep 11, 2016

bgruening commented Nov 13, 2016

bgruening commented Nov 13, 2016

jakirkham commented Nov 14, 2016

bgruening commented Nov 14, 2016

bgruening commented Nov 14, 2016

tomkinsc commented Nov 14, 2016

tomkinsc commented Nov 14, 2016

bgruening commented Nov 14, 2016

jakirkham commented Nov 14, 2016

bgruening commented Nov 14, 2016

jakirkham commented Nov 14, 2016

bgruening commented Nov 14, 2016

jakirkham commented Nov 14, 2016 • edited Loading

tomkinsc commented Nov 14, 2016

conda-forge-linter commented Nov 15, 2016

conda-forge-linter commented Nov 15, 2016

tomkinsc commented Nov 15, 2016

jakirkham commented Nov 15, 2016

tomkinsc commented Nov 15, 2016

jakirkham commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgruening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgruening commented Dec 1, 2016

conda-forge-linter commented Dec 2, 2016

bgruening commented Dec 3, 2016

jjhelmus commented Dec 4, 2016 • edited Loading

jjhelmus commented Dec 4, 2016

tomkinsc commented Dec 4, 2016

Choose a reason for hiding this comment

tomkinsc commented Sep 6, 2016 •

edited by jakirkham

Loading

tomkinsc commented Sep 6, 2016 •

edited

Loading

jakirkham commented Nov 14, 2016 •

edited

Loading

jjhelmus commented Dec 4, 2016 •

edited

Loading