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

Stop using deprecated add_stylesheet in sphinx #1050

Merged
merged 1 commit into from
Jul 2, 2021
Merged

Conversation

yuvipanda
Copy link
Collaborator

add_stylesheet has been deprecated for a while.

This should hopefully fix the CI failures in circleci

@yuvipanda yuvipanda requested review from betatim and manics June 29, 2021 09:44
@yuvipanda
Copy link
Collaborator Author

A different CI step is failing, but at least CircleCI is fixed!

@betatim
Copy link
Member

betatim commented Jun 29, 2021

This is actually a fairly annoying failure. If we merge this PR without fixing it I think we won't get a new image pushed to docker hub, but we wil lget an automated PR for mybinder.org-deploy, which if we merge it will lead to errors because the builder image doesn't exist.

It looks like the error is around ruamel.yaml for which there are no wheels that work on alpine. I have a vague memory of us having a problem like this before, but I don't remember the solution. The only thing I remember is to be wary of ruamel.yaml updates/changes :-/

@betatim
Copy link
Member

betatim commented Jun 29, 2021

The obvious fix is to add gcc or "build essentials" (?) to the build stage docker image. However I have a feeling that the fact that the build doesn't just work is a sign of something else being broken. So we can fix the symptom by adding build tools but we won't be fixing the underlying issue.

At some point we discussed using alpine vs a slim ubuntu/debian image. A switch would probably also ease the pain here, because debian/ubuntu is less "outlandish" than alpine. However, I think we decided that the added pain of using alpine was worth it because we get significantly smaller images. Even if you consider layer sharing between the repo2docker image and the images built by repo2docker. This means we should "stick" with Alpine.

@betatim
Copy link
Member

betatim commented Jun 29, 2021

@yuvipanda can you add build-base to the apk command for the build stage of Dockerfile in the root directory? I think this will fix the build

@yuvipanda
Copy link
Collaborator Author

@betatim I already made a separate PR addressing it :) #1051

add_stylesheet has been deprecated for a while.

This should hopefully fix the CI failures in circleci
@yuvipanda
Copy link
Collaborator Author

I've rebased this, hopefully that should make it pass all the tests

@manics
Copy link
Member

manics commented Jul 2, 2021

Does the minimum version of Sphinx need to be bumped, currently we claim almost any version is supported:

sphinx>=1.4, !=1.5.4

In jupyterhub/mybinder.org-deploy#1914 I pinned all versions exactly and now dependabot takes care of testing upgrades, would it be worth doing the same here?

@yuvipanda
Copy link
Collaborator Author

@manics full pinning + dependabot seems pretty awesome. However, for this PR, I have just changed the pin to a minimum of 1.8 - that's when add_css_file was introduced - this lets us unbreak our green CI check marks!

@yuvipanda
Copy link
Collaborator Author

Would you be willing to do the pinning + dependabot setup here too? Would be sweet! Will provide review + merges if necessary!

@manics
Copy link
Member

manics commented Jul 2, 2021

I opened another PR with the pinning, then realised you'd already opened this one 😄 #1052

I'll merge this, and open a follow-up PR instead.

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

Successfully merging this pull request may close these issues.

3 participants