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.

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

dev-util/nvidia-cuda-{sdk,toolkit}: Version bump to 8.0.44 #2445

Closed
wants to merge 2 commits into from

Conversation

marbre
Copy link
Contributor

@marbre marbre commented Oct 1, 2016

@SoapGentoo

This closes Gentoo Bug 595640 and https://github.com/gentoo-science/sci/issues/688. I'll adopted my release candidate ebuild from the overlay, droped the fetch restriction and further modified some lines. The required drivers is actually a guess, since Nvidia ships 367.48 which is not yet released. Anyway, with 367.44 deviceQuery from the sdk works.
Further, gcc 5.4 might be supported as bosted at parallelforall, but the currently shipped installation docs do not explicitly mention this.

# dodoc doc/*txt
if use doc; then
dodoc doc/pdf/*
dohtml -r doc/html/*
Copy link
Member

@SoapGentoo SoapGentoo Oct 1, 2016

Choose a reason for hiding this comment

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

dohtml is deprecated in EAPI=6. Use the HTML_DOCS variable + einstalldocs instead. Consider doing the same with DOCS too (i.e. DOCS=( doc/pdf/. ))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already played around with this. The result is that either the directory structure gets messed up (no html subdirs anymore) or no documentation gets installed at all. I somehow don't get it..

Copy link
Member

Choose a reason for hiding this comment

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

have you just tried

if use doc; then
    DOCS+=( doc/pdf/. )
    HTML_DOCS+=( doc/html/. )
fi
einstalldocs

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting DOCS and calling default at the end of the phase does not work, because files are previously cleaned up. Calling dodoc instead of dohtml messes up the file structure. Thus any hint which won't needs to redo the complete phase is appreciated. Thus I just left dohtml, since its depreciated but still doing its job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed calling einstalldocs and as mentioned calling default was not helping. Thanks for the hint, fixed now.

Copy link
Member

Choose a reason for hiding this comment

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

'messes up file structure' -- sounds like you were doing something wrong. HTML_DOCS doesn't do anything except for calling dodoc -r

if use profiler; then
# hack found in install-linux.pl
for j in nvvp nsight; do
cat > bin/${j} <<- EOF
Copy link
Member

Choose a reason for hiding this comment

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

missing || die

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done within new commit.

UBUNTU_MENUPROXY=0 LIBOVERLAY_SCROLLBAR=0 \
${ecudadir}/lib${j}/${j} -vm ${EPREFIX}/usr/bin/java
EOF
chmod a+x bin/${j}
Copy link
Member

Choose a reason for hiding this comment

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

missing || die. Consider using fperms from Gentoo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the missing die.

fi

for i in ${remove}; do
ebegin "Cleaning ${i}..."
Copy link
Member

Choose a reason for hiding this comment

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

indent this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done within new commit.

dodir ${cudadir}
mv * "${ED}"${cudadir} || die

cat > "${T}"/99cuda <<- EOF
Copy link
Member

Choose a reason for hiding this comment

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

missing || die

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done within new commit.

ewarn "or append --compiler-bindir= pointing to a gcc bindir like"
ewarn "--compiler-bindir=${EPREFIX}/usr/*pc-linux-gnu/gcc-bin/gcc${b}"
ewarn "to the nvcc compiler flags"
echo
Copy link
Member

Choose a reason for hiding this comment

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

use ewarn instead of echo in order not to mix different output streams

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

b=${a[${#a[@]}-1]}

# if gcc and if not gcc-version is at least greatesst supported
if [[ $(tc-getCC) == *gcc* ]] && \
Copy link
Member

Choose a reason for hiding this comment

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

use the new tc-is-gcc from toolchain-funcs here instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow done, hopefully the right way. Since I only have gcc 4.9 installed I cannot test this, due to modifying the cuda_supported_gcc variable.

doman doc/man/man*/*

use debugger || remove+=" bin/cuda-gdb extras/Debugger extras/cuda-gdb-${PV}.src.tar.gz"
( use profiler || use eclipse ) || remove+=" libnsight"
Copy link
Member

Choose a reason for hiding this comment

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

use a normal if here please, these ( ) subshell thingies aren't nice to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done within new commit.

@monsieurp monsieurp added version bump assigned PR successfully assigned to the package maintainer(s). labels Oct 1, 2016
# dodoc doc/*txt
if use doc; then
dodoc doc/pdf/*
dohtml -r doc/html/*
Copy link
Member

Choose a reason for hiding this comment

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

have you just tried

if use doc; then
    DOCS+=( doc/pdf/. )
    HTML_DOCS+=( doc/html/. )
fi
einstalldocs

?

@marbre marbre force-pushed the nvidia-cuda branch 2 times, most recently from 588f576 to 70e182e Compare October 2, 2016 10:07
Copy link
Member

@mgorny mgorny left a comment

Choose a reason for hiding this comment

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

Next round once the current set of comments is addressed. Please also fix the missing quotes, wrong find uses, mis-indentations and other minor style issues in both ebuilds.

LICENSE="CUDPP"
SLOT="0"
KEYWORDS="~amd64 ~amd64-linux"
IUSE="debug +doc +examples opencl +cuda"
Copy link
Member

Choose a reason for hiding this comment

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

Sort this by name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done within new commit.

media-libs/freeglut
examples? (
media-libs/freeimage
media-libs/glew:*
Copy link
Member

Choose a reason for hiding this comment

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

How do examples support multiple versions of glew?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked the compiled examples with ldd and greped for 'glew', which is not linked. Since glew headers are also shipped with GL, I assume that glew is linked statically. Thus a recompile when glew is updated shouldn't be needed. Please correct me if my assumptions are wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Static linking is almost always a very bad idea. Even if that happens, you really want to rebuild every time glew is upgraded, in case the old version was vulnerable or buggy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, changed it to :=. Hopefully, I one day get the thing with the slotted dependencies, thanks four your patience.


src_unpack() {
unpacker
unpacker run_files/cuda-samples*run
Copy link
Member

Choose a reason for hiding this comment

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

Why are you calling unpacker twice?

Copy link
Member

@mgorny mgorny Oct 2, 2016

Choose a reason for hiding this comment

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

Also, you should check the underlying compression. If it happens to be xz, then you need to dep on xz-utils.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because you need to unpack the archive which includes several further archives. Archives are compressed using gzip.

Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment about that because otherwise it looks like a mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

# epatch "${FILESDIR}"/${P}-asneeded.patch

sed \
-e 's:-O2::g' \
Copy link
Member

Choose a reason for hiding this comment

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

-O[23]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-O3 is removed in the next line of code. Of cause one could make a single line of code of it, but I think this is not a must.

find common/inc/GL -delete || die
find . -type f -name "*\.a" -delete || die

default
Copy link
Member

Choose a reason for hiding this comment

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

Does it have a configure script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume the default call is not necessary, thus should I drop it?

Copy link
Member

Choose a reason for hiding this comment

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

If it doesn't have a configure script, then remove the default ;-).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After removing it, I was reminded why I added it ;) Replaced it by eapply_user as either this or default has to be called.


if use doc; then
ebegin "Installing docs ..."
treecopy $(find -type f \( -name readme.txt -o -name "*.pdf" \)) "${ED}"/usr/share/doc/${PF}/
Copy link
Member

Choose a reason for hiding this comment

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

Also, don't indent on ebegin. It's confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also if it is followed by an eend? I actually think I was told so in gentoo-sci.

eend

ebegin "Moving files..."
for f in $(find .); do
Copy link
Member

Choose a reason for hiding this comment

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

Again, while read loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

doins ${f}
fi
fi
done
Copy link
Member

Choose a reason for hiding this comment

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

Mis-indent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

ebegin "Moving files..."
for f in $(find .); do
local t="$(dirname ${f})"
if [[ ${t/obj\/} != ${t} || ${t##*.} == a ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Why are you doing some crazy substitution instead of == with patterns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jlec ?

if [[ ${t/obj\/} != ${t} || ${t##*.} == a ]]; then
continue
fi
if [[ ! -d ${f} ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Err, -type f to find?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the better way.. Done.


if use doc; then
ebegin "Installing docs ..."
treecopy $(find -type f \( -name readme.txt -o -name "*.pdf" \)) "${ED}"/usr/share/doc/${PF}/
Copy link
Member

Choose a reason for hiding this comment

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

Preferably by using "${ED%/}"/bla instead of "${ED}"bla

@marbre
Copy link
Contributor Author

marbre commented Oct 3, 2016

@mgorny @SoapGentoo Actually, its getting a little hard to bump ebuilds via pull request if there are thus many changes requested. Taking a look into what is in the tree, the ebuilds are already improved, cleaned up e.g. Thus I really don't know if I can spend enough time to take care for all requested changes, as I am also taking care for a bunch of ebuilds in the science overlay and mainly all jupyter ebuilds in the tree.

Gentoo-bug: 595640

Package-Manager: portage-2.2.28
Gentoo-bug: 595366

Package-Manager: portage-2.2.28
@marbre
Copy link
Contributor Author

marbre commented Oct 12, 2016

@mgorny @SoapGentoo Most issues are resolved, hopefully in an acceptable way. The issues persists for ALL other cuda ebuilds in the tree which I didn't touched. Didn't changed a few thinks in the 8.0.44 ebuilds, since I don't know if there originally was a reason to implement it that way and, probably due to a lack of time, @jlec didn't yet respond.

@gentoo-repo-qa-bot
Copy link
Collaborator

@SoapGentoo SoapGentoo self-assigned this Oct 18, 2016
@marbre
Copy link
Contributor Author

marbre commented Oct 24, 2016

@SoapGentoo How can we step forward to get this finally merged?

gentoo-bot pushed a commit that referenced this pull request Oct 26, 2016
Gentoo-bug: 595366

Package-Manager: portage-2.2.28
Closes: #2445

Signed-off-by: David Seifert <soap@gentoo.org>
@SoapGentoo
Copy link
Member

@marbre I've cleaned up the ebuilds and squashed them into your commits. I'm still not absolutely happy with the ebuilds, but they're in a passable state now. Please have a look at how I changed them (especially using bash arrays where possible). Let's try and keep them clean, lean and QA compliant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s).
Projects
None yet
5 participants