-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add Package dSFMT-2.2.3 #3965
Add Package dSFMT-2.2.3 #3965
Conversation
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/dsfmt:
|
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 ( |
recipes/dsfmt/meta.yaml
Outdated
|
||
requirements: | ||
build: | ||
- gcc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would change this to toolchain
. We should be able to build this with the system included compilers I think.
recipes/dsfmt/meta.yaml
Outdated
|
||
build: | ||
number: 0 | ||
script: build.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this script
line. conda-build
does this automatically.
recipes/dsfmt/build.sh
Outdated
|
||
export CFLAGS="-I$PREFIX/include" | ||
export LDFLAGS="-L$PREFIX/lib" | ||
export CPATH=${PREFIX}/include |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be handled by toolchain
.
recipes/dsfmt/build.sh
Outdated
make std-check | ||
make sse2-check | ||
|
||
cp dSFMT.h ${PREFIX}/include |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No make install
? Is there a library or something that needs to be installed as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Makefile
doesn't have an install
target. In the dsfmt.mk
file in the julia repo I see that they build a library:
$(BUILDDIR)/dsfmt-$(DSFMT_VER)/build-compiled: $(BUILDDIR)/dsfmt-$(DSFMT_VER)/source-extracted
cd $(dir $<) && \
$(CC) $(CPPFLAGS) $(DSFMT_CFLAGS) $(LDFLAGS) dSFMT.c -o libdSFMT.$(SHLIB_EXT)
But I don't see any way to build the library directly from the Makefile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That translates to roughly:
gcc -O3 -finline-functions -fomit-frame-pointer -DNDEBUG -DDSFMT_MEXP=19937 \
-fPIC -fno-strict-aliasing --param max-inline-insns-single=1800 -Wmissing-prototypes \
-Wall -std=c99 -shared dSFMT.c -o libdSFMT.so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Might need to create the directory first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, let's add it then. We may come back and tweak it later.
recipes/dsfmt/build.sh
Outdated
|
||
make std | ||
make std-check | ||
make sse2-check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No make check
or make test
step for all of these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make check
just makes a copy of a bash script check.sh
and makes it executable
recipes/dsfmt/build.sh
Outdated
export LDFLAGS="-L$PREFIX/lib" | ||
export CPATH=${PREFIX}/include | ||
|
||
make std |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do? Is there a general make
call for building?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply calling make
has the same result as calling make std
. It builds an executable called test-std-M19937
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. So guess we need to build and install the library ourselves.
recipes/dsfmt/meta.yaml
Outdated
license_file: LICENSE.txt | ||
summary: 'Double precision SIMD-oriented Fast Mersenne Twister' | ||
|
||
# The remaining entries in this section are optional, but recommended |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please drop comments held over from the example
recipe.
recipes/dsfmt/build.sh
Outdated
|
||
gcc -O3 -finline-functions -fomit-frame-pointer -DNDEBUG -DDSFMT_MEXP=19937 \ | ||
-fPIC -fno-strict-aliasing --param max-inline-insns-single=1800 -Wmissing-prototypes \ | ||
-Wall -std=c99 -shared dSFMT.c -o libdSFMT.so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change .so
to ${SHLIB_EXT}
.
recipes/dsfmt/build.sh
Outdated
|
||
mkdir ${PREFIX}/lib | ||
mkdir ${PREFIX}/include | ||
cp libdSFMT.so ${PREFIX}/lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change .so
to ${SHLIB_EXT}
.
recipes/dsfmt/meta.yaml
Outdated
sha256: {{ sha256 }} | ||
|
||
build: | ||
number: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add
skip: true # [win]
recipes/dsfmt/meta.yaml
Outdated
test: | ||
commands: | ||
- test -f "${PREFIX}/include/dSFMT.h" | ||
- test -f "${PREFIX}/lib/libdSFMT.so" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change .so
to ${SHLIB_EXT}
.
This is missing the patches that are needed by Julia to make the necessary functions externally visible from the shared library and use word-sized integer arguments. |
Thanks @tkelman I did make note of that in the issue on the julia-feedstock repo that suggests using this package: |
recipes/dsfmt/build.sh
Outdated
-Wall -std=c99 -shared dSFMT.c -o libdSFMT.${SHLIB_EXT} | ||
|
||
mkdir ${PREFIX}/lib | ||
mkdir ${PREFIX}/include |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally I do mkdir -p
just in case it is already there. Not that it would be in this case. We just don't want to waste time debugging something we are doing just in case these directories are not there.
Honestly this is looking pretty good. We still need to sort out the changes needed for Julia, but think this is largely in a good place. |
The last release from upstream was 4 years ago. The patches are about as old I suspect, would have to go hunting in blame to find who added them initially and if there was any discussion about upstreaming at the time. Didn't realize the code is hosted on github now too, so sending those as patches might be worthwhile if it wasn't done before. |
recipes/dsfmt/build.sh
Outdated
make std-check | ||
make sse2-check | ||
|
||
gcc -O3 -finline-functions -fomit-frame-pointer -DNDEBUG -DDSFMT_MEXP=19937 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ${CC}
here instead of gcc
recipes/dsfmt/meta.yaml
Outdated
test: | ||
commands: | ||
- test -f "${PREFIX}/include/dSFMT.h" | ||
- test -f "${PREFIX}/lib/libdSFMT.${SHLIB_EXT}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a .
here as SHLIB_EXT
already has one.
Thanks @dfornika. Made a few small changes and merged to get this in. Hope that is ok. We can address anything else in the feedstock. |
Raised as issue ( conda-forge/dsfmt-feedstock#1 ) on the feedstock. |
Addresses #3964.