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

Don't require npm to build from source #1039

Merged
merged 9 commits into from
Mar 15, 2021
Merged

Don't require npm to build from source #1039

merged 9 commits into from
Mar 15, 2021

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jan 20, 2021

We already have this scripts in package.json,
and we commit the static files in the repo.

Close #1037

We already have this scripts in package.json,
and we commit the static files in the repo.

Close #1014
@stsewd stsewd requested review from agjohnson and a team January 20, 2021 18:59
@adamjstewart
Copy link

This also closes #1014

@stsewd
Copy link
Member Author

stsewd commented Feb 22, 2021

Looks like warnings are shown when running setup.py install|build, but when running pip install, they are only shown with pip install -v.

more info at pypa/pip#2933 (comment)

@agjohnson
Copy link
Collaborator

I removed closing on #1014, yhis will not close #1014 as we'll want to come back to that issue to resolve the underlying bug with our source builds. For now we're only putting a hack in place.

setup.py Show resolved Hide resolved
Copy link
Member

@Blendify Blendify left a comment

Choose a reason for hiding this comment

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

Nice, to show that this works, you can try searching the docs with the PR build and Latest

@agjohnson
Copy link
Collaborator

For clarification, what we have decided to do here is to communicate deprecation of installation via Git for now, and separate the build step. It seems like the issue in #1037 is that projects are installing through Git -- an installation channel we don't want to support. We don't want to support this as rebuilding assets makes PRs conflict, and we're tied to making release decisions that also include people install unpinned versions of our theme.

For now, the build assets step will be moved, but this PR will be partially reverted later. We'll move back to removing static assets checked into this repository, and all builds should go through an official release.

Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

This is a temporary fix, we need to next figure out deprecation plans.

@stsewd stsewd merged commit f8e71bf into master Mar 15, 2021
@stsewd stsewd deleted the dont-require-npm branch March 15, 2021 15:12
chancez pushed a commit to chancez/sphinx_rtd_theme that referenced this pull request Mar 4, 2022
pchaigno pushed a commit to cilium/sphinx_rtd_theme that referenced this pull request Mar 10, 2022
pchaigno added a commit to cilium/sphinx_rtd_theme that referenced this pull request Mar 11, 2022
This reverts commit c40686d.

This commit and the subsequent one broke the search bar on our hosted
documentation.

Signed-off-by: Paul Chaignon <paul@cilium.io>
qmonnet pushed a commit to cilium/sphinx_rtd_theme that referenced this pull request Mar 11, 2022
This reverts commit c40686d.

This commit and the subsequent one broke the search bar on our hosted
documentation.

Signed-off-by: Paul Chaignon <paul@cilium.io>
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.

Search does not terminate for terms that are not found.
4 participants