Skip to content
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

Make requirements, test requirements more explicit; add [test] pip extra #665

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ryan-williams
Copy link
Contributor

(factored out of #612)

@ryan-williams ryan-williams mentioned this pull request Aug 24, 2021
requirements-test.txt Outdated Show resolved Hide resolved
@savingoyal
Copy link
Collaborator

@ryan-williams If you can address the comment in the PR, we can merge this in.

@ryan-williams
Copy link
Contributor Author

just rebased and addressed the comment, gonna try to resuscitate a few of these, thanks for your patience

@ryan-williams
Copy link
Contributor Author

Just rebased this, not sure what this failure in "R tests on macos-latest" is about:

==> Pouring checkbashisms--2.21.7.all.bottle.tar.gz
installer: Package name is gfortran
installer: Installing at base path /
installer: The install was successful.
/usr/bin/sudo hdiutil detach /Volumes/gfortran-8.2-Mojave
hdiutil: couldn't unmount "disk2" - Resource busy
Error: Failed to get R 4.1.1: Failed to get R 4.1.1: Failed to umount /Volumes/gfortran-8.2-Mojave: Error: The process '/usr/bin/sudo' failed with exit code 16
🍺  /usr/local/Cellar/checkbashisms/2.21.7: 6 files, 72.2KB

@ryan-williams
Copy link
Contributor Author

I think the above was a spurious failure, fwiw; I tweaked this PR once more to alphabetize requirements-test.txt, mostly just to get the CI to run again, and it passed.

So, this should be ready for another look if anyone has time.

savingoyal
savingoyal previously approved these changes Jan 8, 2022
@ryan-williams
Copy link
Contributor Author

thanks @savingoyal 🙏 looks like this is good to merge?

romain-intel
romain-intel previously approved these changes Jan 10, 2022
Copy link
Contributor

@romain-intel romain-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nflx-mf-bot
Copy link
Collaborator

Testing[44] @ 5da5e67

@nflx-mf-bot
Copy link
Collaborator

Testing[44] @ 5da5e67

@nflx-mf-bot
Copy link
Collaborator

Testing[44] had 3 FAILURES.

@ryan-williams
Copy link
Contributor Author

Not sure how to parse "Testing[44] had 3 FAILURES." above. Where does that CI run?

@romain-intel
Copy link
Contributor

@ryan-williams : I am using your PR as a guinea pig for some tests we run internally. The CI is not accessible so don't worry about the message (it's a good comment though -- I will modify the message to indicate that the message can be ignored mostly). As you can see, it's not yet fully working (it worked fine when using branches from this repo but not so much from a fork (yet)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants