-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Check for changes then forcibly mv in javareconf.in #84
Check for changes then forcibly mv in javareconf.in #84
Conversation
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 ( |
${SED-sed} -e "s|JAVA =.\{0,\}|JAVA = $JAVA|" -e "s|JAVA_HOME =.\{0,\}|JAVA_HOME = ${JAVA_HOME}|" -e "s|: \${JAVA_HOME=.\{1,\}|: \${JAVA_HOME=${JAVA_HOME}}|" -e "s|: \${R_JAVA_LD_LIBRARY_PATH=.\{1,\}|: \${R_JAVA_LD_LIBRARY_PATH=${JAVA_LD_LIBRARY_PATH_SH}}|" -e "s|JAVA_LIBS =.\{0,\}|JAVA_LIBS = ${JAVA_LIBS}|g" -e "s|JAVA_LD_LIBRARY_PATH =.\{0,\}|JAVA_LD_LIBRARY_PATH = ${JAVA_LD_LIBRARY_PATH}|" -e "s|JAVAC =.\{0,\}|JAVAC = $JAVAC|" -e "s|JAVAH =.\{0,\}|JAVAH = $JAVAH|" -e "s|JAR =.\{0,\}|JAR = $JAR|" -e "s|JAVA_CPPFLAGS =.\{0,\}|JAVA_CPPFLAGS = ${JAVA_CPPFLAGS}|g" "${file}" > "${file}.new" | ||
if test -f "${file}.new"; then | ||
- mv "${file}.new" "${file}" | ||
+ if test -f "${file}"; then | ||
+ rm -rf "${file}" >/dev/null 2>&1 |
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.
@mingwandroid, in which cases do you think the rm -rf ... ; mv -f
is necessary, rather than just a simple mv -f
?
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.
Should be fine. What do you think to using instead of .new using .$$? doesn't work so well in these docker days sometimes but still worth doing too I think?
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'm not sure. I'll have to think about it for a minute. I guess it should be fine.
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.
Yeah, I'll update it to use $$
. No harm in doing so, I guess.
NB: this isn't tested at all. |
${SED-sed} -e "s|JAVA =.\{0,\}|JAVA = $JAVA|" -e "s|JAVA_HOME =.\{0,\}|JAVA_HOME = ${JAVA_HOME}|" -e "s|: \${JAVA_HOME=.\{1,\}|: \${JAVA_HOME=${JAVA_HOME}}|" -e "s|: \${R_JAVA_LD_LIBRARY_PATH=.\{1,\}|: \${R_JAVA_LD_LIBRARY_PATH=${JAVA_LD_LIBRARY_PATH_SH}}|" -e "s|JAVA_LIBS =.\{0,\}|JAVA_LIBS = ${JAVA_LIBS}|g" -e "s|JAVA_LD_LIBRARY_PATH =.\{0,\}|JAVA_LD_LIBRARY_PATH = ${JAVA_LD_LIBRARY_PATH}|" -e "s|JAVAC =.\{0,\}|JAVAC = $JAVAC|" -e "s|JAVAH =.\{0,\}|JAVAH = $JAVAH|" -e "s|JAR =.\{0,\}|JAR = $JAR|" -e "s|JAVA_CPPFLAGS =.\{0,\}|JAVA_CPPFLAGS = ${JAVA_CPPFLAGS}|g" "${file}" > "${file}.new" | ||
if test -f "${file}.new"; then | ||
- mv "${file}.new" "${file}" | ||
+ if test -f "${file}"; then | ||
+ rm -rf "${file}" >/dev/null 2>&1 | ||
+ fi | ||
+ mv -f "${file}.new" "${file}" | ||
else | ||
echo "*** cannot create ${file}.new~*** Please run as root if required.~" | ${SED-sed} -e 'y/~/\n/' >&2 |
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.
...~" | ${SED-sed} -e 'y/~/\n/' >&2
Just saw that now -- made me laugh :D.
@conda-forge-admin, please rerender |
…da-forge-pinning 2019.06.09
Nice @mbargell, many thanks as always. I'll try to test it out, but does anyone have good pointers for how to reproduce this? I have no NFS, but regardless, I don't think this will make things any worse. |
The easiest way to test this is to use something like snakemake to invoke a bunch of R processes (2 will usually suffice) at once. |
It can be reproduced on Linux with conda create -qynr -c conda-forge r-rjava
num_processes=48 ; seq "${num_processes}" | xargs -P"${num_processes}" -L1 bash -lc 'conda activate r && R -e "library(rJava)" 2>&1 1>/dev/null' You may need to adjust It runs just as I expected with the new patch:
|
@pbordron, @RodrigoGM, @lecorguille, @NTLx, @Fedorov113, @Finesim97, @valscherz, @johanneskoester, @epruesse, @dpryan79: ${SED-sed} -e "s|JAVA =.\{0,\}|JAVA = $JAVA|" -e "s|JAVA_HOME =.\{0,\}|JAVA_HOME = ${JAVA_HOME}|" -e "s|: \${JAVA_HOME=.\{1,\}|: \${JAVA_HOME=${JAVA_HOME}}|" -e "s|: \${R_JAVA_LD_LIBRARY_PATH=.\{1,\}|: \${R_JAVA_LD_LIBRARY_PATH=${JAVA_LD_LIBRARY_PATH_SH}}|" -e "s|JAVA_LIBS =.\{0,\}|JAVA_LIBS = ${JAVA_LIBS}|g" -e "s|JAVA_LD_LIBRARY_PATH =.\{0,\}|JAVA_LD_LIBRARY_PATH = ${JAVA_LD_LIBRARY_PATH}|" -e "s|JAVAC =.\{0,\}|JAVAC = $JAVAC|" -e "s|JAVAH =.\{0,\}|JAVAH = $JAVAH|" -e "s|JAR =.\{0,\}|JAR = $JAR|" -e "s|JAVA_CPPFLAGS =.\{0,\}|JAVA_CPPFLAGS = ${JAVA_CPPFLAGS}|g" "${file}" > "${file}.new"
if test -f "${file}.new"; then
if test -f "${file}"; then
rm -rf "${file}" >/dev/null 2>&1
fi
mv -f "${file}.new" "${file}"
else
echo "*** cannot create ${file}.new~*** Please run as root if required.~" | ${SED-sed} -e 'y/~/\n/' >&2
exit 1
fi with ${SED-sed} -e "s|JAVA =.\{0,\}|JAVA = $JAVA|" -e "s|JAVA_HOME =.\{0,\}|JAVA_HOME = ${JAVA_HOME}|" -e "s|: \${JAVA_HOME=.\{1,\}|: \${JAVA_HOME=${JAVA_HOME}}|" -e "s|: \${R_JAVA_LD_LIBRARY_PATH=.\{1,\}|: \${R_JAVA_LD_LIBRARY_PATH=${JAVA_LD_LIBRARY_PATH_SH}}|" -e "s|JAVA_LIBS =.\{0,\}|JAVA_LIBS = ${JAVA_LIBS}|g" -e "s|JAVA_LD_LIBRARY_PATH =.\{0,\}|JAVA_LD_LIBRARY_PATH = ${JAVA_LD_LIBRARY_PATH}|" -e "s|JAVAC =.\{0,\}|JAVAC = $JAVAC|" -e "s|JAVAH =.\{0,\}|JAVAH = $JAVAH|" -e "s|JAR =.\{0,\}|JAR = $JAR|" -e "s|JAVA_CPPFLAGS =.\{0,\}|JAVA_CPPFLAGS = ${JAVA_CPPFLAGS}|g" "${file}" | cmp -s - "${file}" || {
${SED-sed} -e "s|JAVA =.\{0,\}|JAVA = $JAVA|" -e "s|JAVA_HOME =.\{0,\}|JAVA_HOME = ${JAVA_HOME}|" -e "s|: \${JAVA_HOME=.\{1,\}|: \${JAVA_HOME=${JAVA_HOME}}|" -e "s|: \${R_JAVA_LD_LIBRARY_PATH=.\{1,\}|: \${R_JAVA_LD_LIBRARY_PATH=${JAVA_LD_LIBRARY_PATH_SH}}|" -e "s|JAVA_LIBS =.\{0,\}|JAVA_LIBS = ${JAVA_LIBS}|g" -e "s|JAVA_LD_LIBRARY_PATH =.\{0,\}|JAVA_LD_LIBRARY_PATH = ${JAVA_LD_LIBRARY_PATH}|" -e "s|JAVAC =.\{0,\}|JAVAC = $JAVAC|" -e "s|JAVAH =.\{0,\}|JAVAH = $JAVAH|" -e "s|JAR =.\{0,\}|JAR = $JAR|" -e "s|JAVA_CPPFLAGS =.\{0,\}|JAVA_CPPFLAGS = ${JAVA_CPPFLAGS}|g" "${file}" > "${file}.$$"
if test -f "${file}.$$"; then
mv -f "${file}.$$" "${file}"
else
echo "*** cannot create ${file}.$$~*** Please run as root if required.~" | ${SED-sed} -e 'y/~/\n/' >&2
exit 1
fi
} |
328e45e
to
911605c
Compare
recipe/build.sh
Outdated
curl --insecure -C - -o ${DLCACHE}/miktex-portable-${MIKTEX_VER}.exe -SLO https://miktex.org/download/ctan/systems/win32/miktex/setup/windows-x86/miktex-portable-${MIKTEX_VER}.exe || true | ||
# From https://miktex.org/download#portable : | ||
# Please note that there is no seperate installer. Just download the standard installer and rename it to miktex-portable.exe. | ||
curl --insecure -C - -o ${DLCACHE}/miktex-portable-${MIKTEX_VER}.exe -SL https://miktex.org/download/ctan/systems/win32/miktex/setup/windows-x86/basic-miktex-${MIKTEX_VER}.exe || true |
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.
@conda-forge/r-base
- I'm a monkey and don't know what I'm doing here, so PTAL ;)
- The old URL didn't work, so I updated it.
- Removed the
-O
flag since it's redundant with-o-
. - The old
miktex-portable-${MIKTEX_VER}.exe
URL scheme doesn't work anymore; now it's justbasic-miktex-${MIKTEX_VER}.exe
or the unversionedmiktex-portable.exe
. - (Temporarily?) Copied the the note from their website to avoid future head scratches about that.
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.
Well, turns out a simple 7za x
doesn't cut it anymore. Now the installer contains an archive with still packed components. Running the installer with --portable
should do it. See https://docs.miktex.org/manual/setupwiz.html.
Thanks a lot @mbargull! |
@johanneskoester, you're welcome! I can imagine you have quite a number of downstream |
I can confirm that this patch works! Tested on my usual setup: cluster execution with snakemake through drmaa, spawning 10-30 jobs in parallel. |
I can also confirm that this fixes the problem (using 100 parallel R jobs via snakemake). |
Very cool! Shall we merge? |
Yes please! |
Is it passing on Windows (not that I care about Windows)? |
Nope. Pushed a shady temporary workaround that should be removed as soon as possible. (Or, if someone is willing to put up the time and nerve, the overall build process on Windows should be made more resilient/with less hacks) |
TexLive seems pretty broken to me. I've spent a while trying to fix it so that it finds it's configuration files but it isn't currently playing ball. Miktex changes the way they distribute their stuff basically every time we rebuild R too. It really is a rock and a hard place. |
# MIKTEX_VER=2.9.7100 | ||
# curl --insecure -C - -o ${DLCACHE}/basic-miktex-${MIKTEX_VER}.exe -SL https://miktex.org/download/ctan/systems/win32/miktex/setup/windows-x86/basic-miktex-${MIKTEX_VER}.exe || true | ||
# echo "Extracting basic-miktex-${MIKTEX_VER}.exe, this will take some time ..." | ||
# ${DLCACHE}/basic-miktex-${MIKTEX_VER}.exe --portable=${PWD} --unattended --no-registry || exit 1 |
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.
This works on a local Windows machine. The progress is displayed with a GUI, though, so tough luck inspecting it on the CI...
The installer does something on the CI, as seen by 6e29463 which outputs:
Extracting basic-miktex-2.9.7100.exe, this will take some time ...
du: cannot access './*': No such file or directory
0 total
========================
11885 ./texmfs
11885 total
========================
66684 ./texmfs
66684 total
========================
147410 ./texmfs
147410 total
========================
244796 ./texmfs
244796 total
========================
373200 ./texmfs
373200 total
========================
426704 ./texmfs
426704 total
========================
437725 ./texmfs
437725 total
========================
437725 ./texmfs
437725 total
========================
...
but it obviously hangs somewhere...
Doesn't sound like fun at all. Maybe it makes sense for you to create come intermediate build packages such that you can use them in |
…da-forge-pinning 2019.06.18
@msarahan: All Windows builds failed with
Since I don't want to delay this PR anymore and am not able/willing to investigate, I just forced the CI to use |
92fb516
to
e59a242
Compare
@conda-forge/r-base: If you are fine with the shady/temporary workarounds in |
Thanks |
I don't have quickly any test to validate this PR. My question is what next? |
@lecorguille Just update r-base in your envs, nothing needs to be rebuilt otherwise. |
And for new envs, they will target this new build? |
In general they should, yes. Conda does some complicated things when deciding which builds to use, but all things being equal it'll take a higher build number (i.e., this one). |
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)addresses (=not completely fixes) gh-67
EDIT: This patch does the following:
rm -f
from the previous patch, so that themv -f
is the only write operation to the final files.mv
=>mv -f
from the previous patch.sed
command's output viacmp -s
against the current file content. If no change is detected, the file is not written to at all..new
to.$$
(i.e., add the PID of the current process instead of the constantnew
string).Point 1. (+2.) and 3. both independently help to reduce/avoid concurrent writes and thus combining them should only reduce the overall failure probability even more.
In the (unlikely) case that the Java configuration did change and thus the
cmp -s
from point 3. fails, point 4. is to avoid concurrent creation of the same temporary files.