-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Rework libarrow activate.sh with finer-grained checks and logging #1065
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 ( |
recipe/activate.sh
Outdated
ln -s "$f" "$WRAPPER_DIR/$(basename "$f")" | ||
symlink="$WRAPPER_DIR/$(basename "$target")" | ||
# Check if symbolic link already exists and points to correct file | ||
if [ -L "$symlink" ] && [ "$(readlink "$symlink")" = "$target" ]; then |
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 is the most important part. Covers every activation after the link is created
recipe/activate.sh
Outdated
# Thus it's safest to delete the symlink in case it already exists. | ||
rm -f "$symlink" | ||
# Check if symlink still exists after trying to delete it | ||
if [ -e "$symlink" ]; then |
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.
Instead of rm + existence check + ln -s
you can create a new unique link with random name and mv
to the final place to make it atomic. In two parallel processes rm
+ -e
can happen the same time and the two ln -s
will still collide.
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 modified it. Is that what you had in mind?
This is the approach suggested by @alippai in <conda-forge#1065 (comment)>
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.
Thanks for the PR! Some comments.
The effective part of conda-forge#1033 seems to be checking writability of `WRAPPER_DIR` as opposed to `GDB_PREFIX`. Replacing `ln -sf` with `rm -f; ln -s` seems to have led to conda-forge#1060.
Set `LIBARROW_ACTIVATE_LOGGING=1` to enable log messages.
The run completed successfully. Now pushing the changes to complete the todo... |
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.
LGTM (for squash merge)
I can't imagine the last batch of commits changing much. Is the following failure of __________________________ test_total_bytes_allocated __________________________
def test_total_bytes_allocated():
> assert pa.total_allocated_bytes() == 0
E assert 1344 == 0
E + where 1344 = <built-in function total_allocated_bytes>()
E + where <built-in function total_allocated_bytes> = pa.total_allocated_bytes
test_array.py:41: AssertionError |
Yes Edit: sorry, the comment field contained some outdated stuff I didn't mean to send 🙈 |
You managed to comprehensively confuse github with two PRs having the exact same HEAD, and now I can't restart the builds. Another reason not to open a new PR unless really necessary. 🙃 |
Sorry about that, I was trying to save time by starting a new CI run before the old one finished. Would it help anything if I pushed an empty commit to the other branch and open-close-cycled the other PR, or something like that? |
No worries, I already fixed it here. That commit will not show up in the squash merge |
Looks like one of the runners is a bit messed up. |
Yeah, I don't even get the dialogue for restarting things. Mainly I was waiting if @xhochy still wanted to have some input here. But let's take this for a spin. :) |
We stumbled upon an issue in our ci which seems to be related to this PR, as yesterdays build seems fine and todays doesn't work anymore 😅
The only change I could see was in the build number of |
Hi @plattenschieber, thanks for the report! Indeed, this is would be a natural consequence of your Bash shell having |
We were checking `CF_LIBARROW_ACTIVATE_LOGGING` without a default value. This led to failure under `set -u`. For original report, see <conda-forge#1065>
We were checking `CF_LIBARROW_ACTIVATE_LOGGING` without a default value. This led to failure under `set -u`. For original report, see <conda-forge#1065 (comment)>.
* Fix unbound variable in activate script We were checking `CF_LIBARROW_ACTIVATE_LOGGING` without a default value. This led to failure under `set -u`. For original report, see <#1065 (comment)>. * Bump build number
We were checking `CF_LIBARROW_ACTIVATE_LOGGING` without a default value. This led to failure under `set -u`. For original report, see <conda-forge#1065 (comment)>.
We were checking `CF_LIBARROW_ACTIVATE_LOGGING` without a default value. This led to failure under `set -u`. For original report, see <conda-forge#1065 (comment)>.
We were checking `CF_LIBARROW_ACTIVATE_LOGGING` without a default value. This led to failure under `set -u`. For original report, see <conda-forge#1065 (comment)>.
We were checking `CF_LIBARROW_ACTIVATE_LOGGING` without a default value. This led to failure under `set -u`. For original report, see <conda-forge#1065 (comment)>.
I expect this to fix #1060.
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)