-
Notifications
You must be signed in to change notification settings - Fork 925
[Travis] Fix python/travis-cargo on macOS #2229
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
Conversation
After 3 tries (I don't sh much), this does properly only run the Travis is green again 🎉 |
@nrc is this the right approach? Or should |
Could it be the path is wrong? (See huonw/travis-cargo#70), the solution would be using |
That's a lot of patches incrementally trying things on Travis that I'll rebase down, but I think this is working properly now. Do note though that the
That's on both Linux and macOS. |
- Ensure python is available travis-ci/travis-ci#2312 - Only pip install --user on linux, macOS is in virtualenv - Set PATH in a os-agnostic manner huonw/travis-cargo#71
@nrc PR is good for merge. Though I should say that my command line fu is still weak; if someone has a better way of accomplishing this I'd be happy to acquiesce to them. This was found via trial and error. |
Scratch that, going to try a couple other options I just ran into on StackOverflow. Since I need to run them on Travis, they're going to end up on the end of this PR. DO NOT MERGE YET |
Well, that didn't work. I thought maybe I guess we're good again. I force pushed to remove the experimentation again. |
.travis.yml
Outdated
else | ||
pip install 'travis-cargo<0.2' --user | ||
fi | ||
- export PATH="$(python -m site --user-base)/bin:$PATH" |
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.
Is the idea here to get the travis-cargo
script onto PATH
? In the virtualenv case (macOS here), travis-cargo
is not in $(python -m site --user-base)/bin
. It's in $VIRTUAL_ENV/bin
. However, you don't need to add anything to the path in this case because virtualenv
modifies the path when you activate it. Thus, you should be able to move this export line into your else
block.
virtualenv
case
# let's peek at PATH before activating the virtualenv
$ echo $PATH
/Users/davidalber/.cargo/bin:<other stuff>
$ virtualenv env -p python3
# (output omitted)
$ . env/bin/activate
# let's take another look at PATH
$ echo $PATH
/Users/davidalber/env/bin:/Users/davidalber/.cargo/bin:<other stuff>
# contrast with VIRTUAL_ENV
$ echo $VIRTUAL_ENV
/Users/davidalber/env
$ ls $VIRTUAL_ENV/bin
activate easy_install pip3.6 python3.6
activate.csh easy_install-3.6 python wheel
activate.fish pip python-config
activate_this.py pip3 python3
# ^^ no `travis-cargo` script
$ pip install 'travis-cargo<0.2'
# (output omitted)
$ ls $VIRTUAL_ENV/bin
activate easy_install pip3.6 python3.6
activate.csh easy_install-3.6 python travis-cargo
activate.fish pip python-config wheel
activate_this.py pip3 python3
# ^^ there it is!
Non-virtualenv
case
I did this in an Ubuntu 16.04 Docker container.
$ python -m site --user-base
/home/myuser/.local
$ ls `python -m site --user-base`/bin
ls: cannot access '/home/myuser/.local/bin': No such file or directory
$ pip install 'travis-cargo<0.2' --user
# (output omitted)
$ ls `python -m site --user-base`/bin
travis-cargo
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.
You are indeed correct. I'm learning on my feet here, and am a Windows user just brute forcing things. This is a clear improvement on what I have, thanks!
as per [@davidalber's advice](#2229 (comment))
.travis.yml
Outdated
pip install 'travis-cargo<0.2' | ||
else | ||
pip install 'travis-cargo<0.2' --user | ||
export PATH="$(python -m site --user-base)/bin:$PATH" |
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.
One other thing worth pointing out is that the before_script
was previously
pip install 'travis-cargo<0.2' --user && export PATH=$HOME/.local/bin:/usr/local/bin:$PATH
This ensured that the export
did not run unless the pip install
succeeded. The changes in this PR break that, and if the pip install
fails, I think (disclosure: I don't have much experience with Travis CI) that Travis CI will keep going after before_script
until something fails due to the absence of travis-cargo
. That seems to be the implied consequence in travis-ci/travis-ci#1066.
There's a similar situation with the macOS branch of the condition in these changes, which has more opportunities to fail.
I believe the right action here is to chain the commands in both branches of the if
statement with &&
. See, for example, rust-lang/rust#12513, where this was done.
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 you wanted to experiment, you could also try putting a set -e
at the beginning of the script and a set +e
at the end. You would do that instead of the &&
chaining, which would be a burden if the script was really long. If you don't want to experiment, the chaining is fine, though, since it's not that long, and it's the convention elsewhere in the file already.
.travis.yml
Outdated
brew install python3 && | ||
virtualenv env -p python3 && | ||
source env/bin/activate && | ||
pip install 'travis-cargo<0.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.
You have a trailing &&
here.
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 believe I'm gating this being merged in any way, but I wanted to note that I am finished commenting on this PR. Assuming the macOS build passes once Travis CI finally schedules it, this looks good to me.
Thanks for putting this together, @CAD97!
This seems to be running forever on Tracis on OS X now. |
@nrc I don't think that's my fault, e.g. https://travis-ci.org/rust-lang-nursery/rustfmt/builds/312591174 on master has been waiting more than 24 hours. https://travis-ci.org/rust-lang-nursery/rustfmt/jobs/312673236 is the macOS build for this PR, and is waiting for queue (e.g. it has not run yet). travis-ci/travis-ci#7304 is the persistent Travis issue about slow to start macOS builds. See also https://www.traviscistatus.com/incidents/95yttvs37v25 (December 2, 2017) and https://www.traviscistatus.com/incidents/1vd05j8y0yfq (December 7, 2017). |
Build's done! |
Thank you @CAD97 for iterating on this and @davidalber for review! I'm really glad our CI is up and working! |
The solution is backported from rustfmt: rust-lang/rustfmt#2229
The solution is backported from rustfmt: * rust-lang/rustfmt#2229 * rust-lang/rustfmt#2504
The solution is backported from rustfmt: * rust-lang/rustfmt#2229 * rust-lang/rustfmt#2504
Builds on macOS are failing, e.g. https://travis-ci.org/rust-lang-nursery/rustfmt/jobs/310698549, due to
pip
not working.This PR
pip install --user
on linux but without--user
on macOS, which has python in virtualenv