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

Build JSON artifacts in HTML builder #4554

Merged
merged 4 commits into from Aug 22, 2018

Conversation

@humitos
Copy link
Member

@humitos humitos commented Aug 22, 2018

This removes the Feature BUILD_JSON_ARTIFACTS_WITH_HTML and makes this the default for all the projects.

It also removes the SearchBuilder since it's not used anymore.

Depends on readthedocs/readthedocs-sphinx-ext#50
Related #4229 and readthedocs/readthedocs-corporate#378

json_path = os.path.abspath(
os.path.join(self.old_artifact_path, '..', 'json')
# Copy JSON artifacts to its own directory
# to keep compatibility with the older builder.
Copy link
Member Author

@humitos humitos Aug 22, 2018

This comment confuses me a little.

What is what we need to keep compatibility to?

Copy link
Member

@ericholscher ericholscher Aug 22, 2018

With the code that copies the build artifacts onto the web servers.

@@ -143,4 +143,4 @@ else:
extensions = ["readthedocs_ext.readthedocs"]

# Build the json artifacts with the html build step
rtd_generate_json_artifacts = {{ generate_json_artifacts }}
rtd_generate_json_artifacts = True
Copy link
Member Author

@humitos humitos Aug 22, 2018

I didn't find anything where we are using this, but removing it may break other things maybe. I don't know.

Copy link
Member

@ericholscher ericholscher Aug 22, 2018

It was added for this, so can be removed.

Copy link
Member

@ericholscher ericholscher Aug 22, 2018

You do need to update the sphinx extension though, so those need to be done together: https://github.com/rtfd/readthedocs-sphinx-ext/blob/master/readthedocs_ext/readthedocs.py#L179

Copy link
Member Author

@humitos humitos Aug 22, 2018

Removed and opened another PR at readthedocs/readthedocs-sphinx-ext#50

Copy link
Member

@ericholscher ericholscher left a comment

👍 💯 Glad to get this rolled out everywhere.

json_path = os.path.abspath(
os.path.join(self.old_artifact_path, '..', 'json')
# Copy JSON artifacts to its own directory
# to keep compatibility with the older builder.
Copy link
Member

@ericholscher ericholscher Aug 22, 2018

With the code that copies the build artifacts onto the web servers.

@@ -143,4 +143,4 @@ else:
extensions = ["readthedocs_ext.readthedocs"]

# Build the json artifacts with the html build step
rtd_generate_json_artifacts = {{ generate_json_artifacts }}
rtd_generate_json_artifacts = True
Copy link
Member

@ericholscher ericholscher Aug 22, 2018

It was added for this, so can be removed.

@@ -143,4 +143,4 @@ else:
extensions = ["readthedocs_ext.readthedocs"]

# Build the json artifacts with the html build step
rtd_generate_json_artifacts = {{ generate_json_artifacts }}
rtd_generate_json_artifacts = True
Copy link
Member

@ericholscher ericholscher Aug 22, 2018

You do need to update the sphinx extension though, so those need to be done together: https://github.com/rtfd/readthedocs-sphinx-ext/blob/master/readthedocs_ext/readthedocs.py#L179

@humitos
Copy link
Member Author

@humitos humitos commented Aug 22, 2018

All tests passed: https://travis-ci.org/rtfd/readthedocs.org/builds/419204927?utm_source=github_status&utm_medium=notification

I don't know why this is not reporting properly to GH.

@stsewd
Copy link
Member

@stsewd stsewd commented Aug 22, 2018

I restarted a job to refresh the state, now it's all green!

humitos added 3 commits Aug 22, 2018
This removes the Feature BUILD_JSON_ARTIFACTS_WITH_HTML and makes this
the default for all the projects.

It also removes the SearchBuilder since it's not used anymore.
@humitos humitos force-pushed the humitos/search/remove-builder-json-feature branch from 43a5cf7 to b6fc199 Aug 22, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Aug 22, 2018

Rebase and ready to merge. Waiting for tests to pass.

@humitos humitos merged commit cab4363 into master Aug 22, 2018
1 check passed
@humitos humitos deleted the humitos/search/remove-builder-json-feature branch Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants