-
Notifications
You must be signed in to change notification settings - Fork 237
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
Reordering the Linux docker commands to get rid of timeout logic #97
Reordering the Linux docker commands to get rid of timeout logic #97
Conversation
aa99cee
to
733b09d
Compare
…y swapping splitting docker run into create and start
733b09d
to
f417a91
Compare
a8e333f
to
4cc09a3
Compare
If this now builds (and I think it should), this idea is fully worked out, so feel free to review and see if it's to be included or not, @joerick and @altendky. Extra note: since the mounts are ignored by CircleCI (as the comment by @altendky says anyway) and since the copy call is now a single line, we could in principle add the extra |
Oh, btw, extra note: the |
Isn't there already a verbosity config that I didn't notice until after? And yes, absolutely should use some logging library instead of my prints... |
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 much clearer, I like it! I just had a few comments.
cibuildwheel/linux.py
Outdated
except KeyboardInterrupt: | ||
process.kill() | ||
process.wait() | ||
return process.returncode |
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 function is a little strange in that if you give it a stdin_str, it doesn't raise on failure. I think maybe it would be better to raise a subprocess.CalledProcessError if the return code is non-zero, then change error handling at the call site
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.
True; let me change that.
cibuildwheel/linux.py
Outdated
script_returncode = run_docker(['start', '-i', '-a', container_name], stdin_str=bash_script) | ||
run_docker(['cp', container_name + ':/output/.', os.path.abspath(output_dir)]) | ||
run_docker(['rm', '-v', container_name]) | ||
|
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.
If we make the error handling to run_docker, this code might be neater as a try: finally:
block, so that if any of the docker commands fail we still clean up the container before exiting.
e.g.
try:
# docker commands - create, cp, run, cp
finally:
run_docker(['rm', '-v', container_name])
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, will do
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.
Actually, if we're already failing anyway, do we really care about cleaning up the docker container?
cibuildwheel/linux.py
Outdated
return subprocess.check_call(['docker'] + command) | ||
else: | ||
process = subprocess.Popen(['docker'] + command, | ||
stdin=subprocess.PIPE, universal_newlines=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.
Just a style thing - I'm not 100% on moving the 'docker' part of the command into this function, because then you have to imagine the 'docker' word when reading the invocations. But happy to leave this if you prefer it as it is
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 know (and kind of expected a reaction on that). The main thing was that it was a bit weird to call the function run_docker
when docker needed to be added manually. And that it would slightly shorten the lines actually using it (since you're already reading run_docker
).
Happy to remove this part again, if you want? Or go ahead and push it yourself to this branch, if that's the only thing remaining after the other fixes.
I don't think the mounts are ignored on Circle, but because they're on a different machine than the test runner they're not much use. e.g. from the log
Yes, we could... but... this feels like increasing our testing surface area a bit to me, opening the possibility of bugs that happen on one platform but not the other. So that's why I haven't been super keen on the idea. But if those copy commands might take more than ~10 seconds, I'd consider it.
Happy for you to remove them for now, if you like. Also in the Circle logs the |
There's the build verbosity, that's passed to
Oh, right. Yeah, no, I think it's fine like this then. I liked the mounts, but ... :-)
If you want? I can also leave them until we clean up further?
And yes, that was the idea, more or less, but I'm not sure I feel like doing that now. |
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.
Good deal, thanks for tidying my modifications.
'-v', '/:/host', # ignored on Circle | ||
docker_image, '/bin/bash']) | ||
run_docker(['cp', os.path.abspath(project_dir) + '/.', container_name + ':/project']) | ||
run_docker(['start', '-i', '-a', container_name], stdin_str=bash_script) |
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.
How about we skip the communicate stuff? Have a real .sh file instead of a string (which could happen regardless) and copy it into, say, /cibuildwheel.sh
and then just have docker run it. Then it can always check_call()
.
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 read your comment on this in the other conversation, and I actually tried out some stuff, but it indeed seems that the only way would be to create a file, copy it into the image, and run it from there. And that seems +- as involved as the current solution? Plus, creating that file and copying it seems ... I don't know. I think a stdin
solution is quite nice (though admittedly, the communicate
step isn't really).
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.
@joerick Any thoughts?
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 think that regardless the bash script should probably be stored in the repository as a .sh file. If the present .communicate()
approach were kept then you'd with open('the_bash.sh') as f: script = f.read()
then .communicate()
the script
string as we do now.
Sure, there are other steps, but...
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.
Hmmm, but what about the CIBW_BEFORE_BUILD
etc, that is inserted in the script template? Are you suggesting to include those in other ways? I guess environment variables could work as well, to insert custom steps. Before, I thought you meant writing the script from Python string to a temporary file, each time, so I hadn't considered this possibility.
Personally, I do actually quite like having all code related to building on Linux in one file, just like it is for macOS and Windows (even though, I admit, it looks kind of ugly to have a full bash script inside a Python string literal).
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.
Alrighty, you got me... I skipped over that detail since I was talking abstractly instead of trying to implement my idea. Still, it could be the same thing it is now but in a file. Then you get syntax highlighting for bash etc (other than issues caused by the substitution markers). But sure, I'll back off a bit. I might still rather write to a temp file but... whatever, I'll back off. :]
In another direction, don't we get Python in the container? Couldn't this be a Python script and it could load a config file that we write?
Obviously none of this is needed, just sharing thoughts at this point.
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.
In communicate
vs temp file, I'm very slightly in favour of communicate, just seems neat not to involve an extra file somehow!
Yeah, the script is bash since I thought it was really confusing to write a python script inside a string literal inside a python file!! So just wrote it in bash to get it done. It would be cool if somehow it was a fully fledged python script though. Maybe then there would be potential to unify the Linux/Mac/windows divide? :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.
@altendky Nono, that wasn't the plan, to shut up the discussion. It was rather to see the pros and cons and compare them for the different options that I was thinking about advantages of the communicate
, not to have you 'back off' or anything! And indeed, actually writing this in Python might be interesting 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.
@YannickJadoul there was no negative connotation intended in my message. :] You honestly caught me pushing an idea that I hadn't thought through and you had found an issue with. Anyways, I think that maybe a good way would be a Python script with a CLI (probably small enough to comfortably do with stdlib and 2/3 compatibility) and we pass in the info via the command. No config file to write and copy, no template to fill out. Just copy the exact Python script over to the container and then docker run it with proper arguments.
That would eliminate bash, templating, communicate, and code in a string literal... Costs include another docker copy (not a big deal?) and the code being in two places (this really doesn't bother me).
Or, maybe we just replace the string/file with python code calling docker run for each command? shrug We could also walk away because it's working fine and we could always do this later. :]
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've created #99 to continue this discussion :)
Thank you for figuring out all of those modifications and doing all the hard work in the first place! |
cibuildwheel/linux.py
Outdated
exit(1) | ||
finally: | ||
# Still gets executed, even when 'exit(1)' gets called | ||
run_docker(['rm', '-v', container_name]) |
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.
Add the --force
option to this? I don't know if it's possible for it still to be running at this point but it can't hurt 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.
Uuuh, right. I think I forgot to transfer that from the old code :-/
(EDIT: Hm, no that was -v
. Either way, I'm adding it)
Any decision on that? I don't have a strong opinion (see comment/answer above), so I'm happy to change it if you prefer. Actually, I first tried |
Lets leave as is - it's very clear what's going on :) Feel free to merge this @YannickJadoul when you're ready. |
I think I'm happy with this PR. But unless you're releasing soon, there's no rush, so we can still wait for @altendky to confirm this doesn't need any more changes? |
Wait... this was pseudo waiting on me? :[ Anyways, trying it out in twisted/twisted#1051. |
Quick check looks ok. @YannickJadoul, looks like you've got the go-aheads. Merge away (and I'm sorry I left this hanging). |
There was no rush to merge this, so don't worry :-) I think the Circle CI changes have not been officially released, anyway, I think, so this can still be part of that same release. Though, with Circle CI and |
AAAaaaaand it's released! Thanks @altendky and @YannickJadoul for all the amazing work you've both put in over the last month to get CircleCI support in! |
Niiiice. Time to start using it! |
See #96