Change how we do packaging

agjohnson commented 7 years ago

Currently, PRs are expected to grunt build themselves. This leads to a merge conflict problem when there are multiple PRs open. We should instead be handling build artifacts on release.

The workflow here would be that when it makes sense, a maintainer will release the package, either as a development release or as a prod package. The maintainer will run grunt build, check those artifacts into the repository, the commit will be tagged, and a release to go out to PyPI.

We can't remove storing artifacts as a number of folks pin their theme dependency here. We can remove the requirement that PRs run grunt build, but perhaps find another mechanism for keeping our version on conflict, such as using .gitattributes to use ours on merge.

jessetan commented 6 years ago

Removing the need for PRs to run grunt build is a good idea to reduce merge conflicts. We could add instructions in a PR template

If building and committing the assets to the repo is needed for people who use master instead of a package from Pypi, this can be a periodic task for maintainers or automated using a bot and/or git webhooks.

agjohnson commented 6 years ago

I think a release time process by maintainers makes the most sense. We can think about automating this later perhaps.

We're hoping to shorten the release cycle and make more minor releases with the theme. With that, we can just release more often, generate build assets on release, and perhaps just disregard the use case of running the theme from master -- running from master would effectively just be the same as running from the latest release.

jessetan commented 6 years ago

Keeping the built assets up to date can be done with Travis as following (roughly):

An after_success block is added to .travis.yml to call a bash script
The script checks if the commit is not a PR commit and the affected branch is master. This is effectively a check for a merged PR.
The script installs any dependencies and runs grunt build to create the build artefacts (located in sphinx_rtd_theme/static)
The script commits these files to master using a properly secured access token

The process for a PR then becomes:

Authors creating a PR will be presented with a PR template that instructs them not to include the built assets (although nothing will break if they do)
Comments and review
PR is merged to master
Travis commits built artefacts to master, making master up to date with the changes from the PR

Inspiration:

https://github.com/cvan/webvr/blob/ca0e903/scripts/deploy.sh https://gist.github.com/domenic/ec8b0fc8ab45f39403dd https://benlimmer.com/2013/12/26/automatically-publish-javadoc-to-gh-pages-with-travis-ci/

This does not solve the problem of releasing the theme, although it could be expanded to help with automating part of it.

Blendify commented 6 years ago

I like this idea.

davidfischer commented 6 years ago

When I reread the comment from @agjohnson, it sounds to me like he's proposing adding the built files to .gitignore and removing them from the repo (git rm). Then these would need to be built right before a release is made. Correct me if I'm wrong. As he mentions this would mean that users couldn't just use the theme from "master". They would have to use a packaged version.

What @jessetan is proposing sounds more like having an automated way to keep the built artifacts up to date. This would still allow users to use the theme from master but at the cost of some complexity.

If nobody has a strong opinion, I prefer the first approach but not too strongly.

jessetan commented 6 years ago

@davidfischer that's correct. A requirement for the first approach would be to do releases more often. This is also has my preference BTW.

jessetan commented 5 years ago

@agjohnson any update on simplifying or speeding up the release process? Supporting a theme install from master has some drawbacks (e.g. #712)

davidfischer commented 5 years ago

For the record, I'm 👍 on removing built artifacts from the repo and only supporting using released versions (not an install from git master). I don't think releasing frequently is a big burden.

agjohnson commented 5 years ago

I am also :+1: on just git ignore on the whole sphinx_rtd_theme/static/ path and making building assets strictly a release process. I'd like to support a single installation path, so :+1: on that too.

Unfortunately, anyone that hit #712 (myself included) would have broken docs pretty quickly. Our docs do currently state you can git install/submodule install, and I did remove these instructions in a PR, but we were communicating a blessed installation path to users by having this in our docs. So, these users aren't wrong for installing like this.

For now, the webpack work is actually going to revert the font path change (I didn't know this was an issue, and it's odd that we take a stance on fonts, but not our other assets) and all assets will be checked in for the time being. It sounds like we need more of a plan around breaking this workflow first?

Could we detect that the user is running a non-released version (compare a non-release package version? test for a file in the repo and not in release? something else in setup.py?). We'll start by throwig a deprecation warning in sphinx and let this simmer for a couple releases perhaps?

agjohnson commented 5 years ago

Also, regarding release schedule: would it help to develop a policy around releases? The policy we have internally boils down to something like:

We ship often on alpha/beta tagged releases -- these won't be picked up by pip automatically, users can select into them still though
We do not ship proper releases unexpectedly -- team is notified before release
No proper releases while the team is not available -- weekends, holidays, etc
Critical releases can go out as needed, mostly disregarding these rules -- critical being defined as security issues, strong breaking behavior on RTD, etc

Would the theme core team be :+1: on something similar? Having some explicit rules around the process helps set expectations and allows for autonomy. Getting more people releasing will reduce release pressure and with the rules above, I'm not worries about shipping breaking changes.

davidfischer commented 5 years ago

Our docs do currently state you can git install/submodule install, and I did remove these instructions in a PR, but we were communicating a blessed installation path to users by having this in our docs.

I see that now in #778. It was in the docs but the README said something different. We should remove that and only support a module install (eg. with pip).

would it help to develop a policy around releases?

I don't think it would hurt but I don't think this is what's holding back releasing more often.

davidfischer commented 5 years ago

Could we detect that the user is running a non-released version (compare a non-release package version? test for a file in the repo and not in release? something else in setup.py?)

The most common pattern I've seen for this is to have the version in master be something like $VERSION-dev and the -dev gets dropped when it is released.

jessetan commented 5 years ago

I'm in favor of removing support for installing from git (and not having compiled assets in the repo), with more frequent -dev releases built from master.

We could have a transition period where we do build assets into the repo and log a deprecation warning to the console if the current version ends with -dev. After this period, we remove the built assets from the repo and change the JS warning to inform users that this is a dev version of the theme.

agjohnson commented 5 years ago

Sounds like a good plan. Webpack will be changing the fonts back to included in repository theme path for now. We can follow these PRs up with a PR for deprecation warnings in docs and on build. When we're ready, built assets will be removed from the repository.

I don't think it would hurt but I don't think this is what's holding back releasing more often.

Having releases blocked by RTD core is probably the biggest piece keeping the theme from releasing often. My point was to have a policy doc to set expectations around theme releases for theme maintainers, not necessarily core RTD team, and allow the theme maintainers to release.

Letme commented 5 years ago

I will join this discussion. I have strong opinion to keep the artefacts out of git and on .gitignore as that avoids the problems described above. Installing from git is also possible using pip install -e path_to_git_repo_and_branch which packages and installs from git repo (any branch). So this should be next advertised method.

agjohnson commented 5 years ago

We're effectively discussing not supporting installation outside of PyPI with this change. Something like pip install -e wouldn't get around the missing build artifacts, and so wouldn't be a prescribed installation method anymore.

If we don't include build artifacts, you will need to install all of the packages development dependencies and build the theme if you want to use it from a git repository or submodule. You can pip install -e after building the theme, but we'd probably view this as out of scope for what the core team supports.

Letme commented 5 years ago

Why would it not get around? pip install -e runs setup.py (with my understanding) and then installs the package. So if we put everything (building, obtaining artifacts, etc) in setup.py, then we should be able to build the theme and install it with pip install -e directly from repository.

I do agree above case brings added dependencies (lets face it, not many people install this way), but if we really want, we can also just push development releases to pypi from for example master.

davidfischer commented 5 years ago

Why would it not get around? pip install -e runs setup.py (with my understanding) and then installs the package. So if we put everything (building, obtaining artifacts, etc) in setup.py, then we should be able to build the theme and install it with pip install -e directly from repository.

Many of these dependencies will be non-Python dependencies. This issue discusses using webpack to compile static assets. I haven't thought about it deeply, but I don't think that setup.py should be making subprocesses to install node packages and run webpack.

davidfischer commented 5 years ago

I don't think that setup.py should be making subprocesses to install node packages and run webpack.

Shows how up-to-date I am. That is currently in setup.py.

Letme commented 5 years ago

Can you guys take a decision which way we go, so that someone can jump on the task and get us back to releasing pace?

agjohnson commented 5 years ago

@Letme we will not be supporting installation from the repository. We do not want to support users using our master branch, which is not even alpha release quality. We will be releasing to PyPI as development releases more often once some of our structural changes are ironed out.

readthedocs / sphinx_rtd_theme

Change how we do packaging #379