-
Notifications
You must be signed in to change notification settings - Fork 90
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
lib/fetch-{libc++,newlib}: exit on error #477
base: master
Are you sure you want to change the base?
Conversation
Seems like we were relying on failures being silently ignored in CI:
I'll push a commit that changes the invocation of |
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 don't think these fetch scripts should be getting called in a way that causes this to be an issue, so I have a mild fear this is papering over something else, but that is an issue for another day.
Oof. Or... it's not an error for another day. Looks like the CI error is competing unzip's trampling each other, likely because the first unzip was only partially complete when the second one first checked if it needed to do anything :/ |
… racing each other
@ppannuto Yuck, unfortunately that assessment is spot on. I ended up implementing some simple flock-based file locking strategy which seems to work (on Linux-based systems at least). Indeed we're running into lock contention in CI:
It's debatable whether the current strategy of waiting on the lock and then fetching and unzipping again is warranted, given that multiple executions of the fetch scripts should be idempotent, but alas. This seems to work, and I'm not very passionate about polishing this stuff any further. 😄 |
@ppannuto Can you perhaps validate that this still works on macOS? |
This helps diagnose issues when one or more of the tools that the script requires are not installed.
In my case, I was working on a system that did not have
unzip
installed. This error message was hidden in between a lot of other log messages, and the error was masked by a generic "can't find this header" message.This also removes the
let
bindings. The bashlet
commands returns exit code 1 when assigning the value0
to a variable, which is ... 100% what you want, just great behavior, and definitely did not cost me another 10 minutes to debug.