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

Move master off of 2.4.4 #57

Open
nicholasphair opened this issue Feb 29, 2024 · 7 comments
Open

Move master off of 2.4.4 #57

nicholasphair opened this issue Feb 29, 2024 · 7 comments

Comments

@nicholasphair
Copy link
Collaborator

#56 added support for different Sphinx versions through tags. master, however, still points to 2.4.4. That was purposeful. It was done to not immediately breaks builds and give people time to kick the tires on the tagging approach.

This issue is for tracking/coming up with a sane policy for moving the the master pointer to something more recent. Perhaps just a message in the logs encouraging folks to pin a specific version.

@yinchi
Copy link

yinchi commented Sep 7, 2024

Perhaps use an input to set the Docker image version? Default to latest if not set.

- name: Sphinx Build
  uses: ammaraskar/sphinx-action@master
    with:
      docs-folder: "docs"
      sphinx-tag: "7.4.7"

This way, the versioning of the Github Action is divorced from the version of Sphinx itself.

To get our new input into the Docker build, we could use Docker Container Action with build-arg. The Dockerfile would then start with:

ARG SPHINX_TAG=latest
FROM sphinxdoc/sphinx:$SPHINX_TAG

Our action.yml would contain something like:

runs:
  using: 'composite'
  steps:
    - uses: pl-strflt/docker-container-action@v1
      with:
        repository: ${{ steps.github.outputs.action_repository }}
        ref: ${{ steps.github.outputs.action_sha || steps.github.outputs.action_ref }}
        dockerfile: Dockerfile
        build-args: |
          SPHINX_TAG:${{ inputs.sphinx-tag }}

@nicholasphair
Copy link
Collaborator Author

Does that build an image each time the action is run? Would that not be slow? If not that could be a nice change.

Just to be clear, in case it is not, you can specify the sphinx version already:

- name: Sphinx Build
  uses: ammaraskar/[email protected]
    with:
      docs-folder: "docs"

Good for end-users, but for maintainers of this project it requires effort to keep dockerfiles and tags in sync with the upstream sphinx project. Something that nobody really has time for and is why the versioning lags behind. That might be good motivation to consider #55. Though, that is certainly not my call.

In any case, the dev branch does have a script and a nightly workflow to try and automate this synchronization process. Maybe that can be merged @ammaraskar.

@ammaraskar
Copy link
Owner

I'm fine with it if you want merge dev into the main branch, I won't be able to review it at the moment.

@yinchi
Copy link

yinchi commented Sep 8, 2024

Unfortunately I couldn't make the suggestions in my previous post work. Will keep trying in my free time. For now I have my own fork with the version pinned to 7.4.7.

@nicholasphair
Copy link
Collaborator Author

Merging the dev branch would move master off 2.4.4 and I don't think anybody wants to deal with that right now. I did cherry-pick some of the tooling and automation though.

The cicd workflow should run nightly so we'll have to wait for a new sphinx release to see if it does its job. If it did, this repo should be kept in synch with sphinx going forward.

I also triggered the synch manually and there are now tagged versions of this action up until 8.0.2. So @yinchi I think you can use ammaraskar/[email protected].

That's about all the bandwidth I have to contribute at this time. Hope it helps.

Oh, and this issue should probably remain open since master is still on 2.4.4.

@yinchi
Copy link

yinchi commented Sep 9, 2024

Does that build an image each time the action is run? Would that not be slow? If not that could be a nice change.

@nicholasphair I don't think it's too slow, since our Dockerfile is just copying a few files over to the base image. The slowest parts appear to be handling requirements.txt if provided, and the actual build (both unavoidable).

The issue with my approach, I guess, is that it doesn't go well with the current tryrelease cronjob.

@ferdnyc
Copy link

ferdnyc commented Oct 14, 2024

Perhaps use an input to set the Docker image version?

My #39 actually does that, as a feature I tacked on at the end. I did it there by writing a separate Dockerfile for each supported tag (named Dockerfile.tagname), so they could be selected using with: / container: tagname; the container to run the action in was specified in its own YAML as image: ${{ format('Dockerfile.{0}', inputs.container) }}.

Downsides

  • Only supports a fixed set of version tags
  • Requires a Dockerfile for each

Advantages

  • Works
  • The Dockerfile for each version can be customized as needed.

(I originally added it in order to base a pdflatex version of the action on the upstream sphinxdoc/sphinx-pdflatex:4.3.2 image, which is a completely separate container image from the various sphinxdoc/sphinx versions. So the Dockerfile.pdflatex source did indeed differ from the other Dockerfiles, in more than just the version tag.)

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

No branches or pull requests

4 participants